Apply percent decoding to resource attributes list - #4460
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4460 +/- ##
==========================================
+ Coverage 82.62% 82.64% +0.02%
==========================================
Files 512 513 +1
Lines 20138 20139 +1
==========================================
+ Hits 16637 16641 +4
+ Misses 3501 3498 -3
🚀 New features to boost your workflow:
|
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the PR! Please see some minor feedback below. Once CI is passing I will approve.
| #include "opentelemetry/version.h" | ||
| #include "src/common/wildcard_match.h" | ||
|
|
||
| #include "../resource/detail/percent_decode.h" |
There was a problem hiding this comment.
can this be src/resource/detail/percent_decode.h?
| opentelemetry::common::AttributeValue wrapped_attribute_value(decoded_value); | ||
| list_attributes.SetAttribute(attribute_key, wrapped_attribute_value); | ||
| } | ||
| else |
There was a problem hiding this comment.
if attribute_valid may still be false please keep this warning.
| if (attribute_valid) | ||
| { | ||
| opentelemetry::common::AttributeValue wrapped_attribute_value(attribute_value); | ||
| std::string decoded_value = opentelemetry::sdk::resource::detail::PercentDecode( |
There was a problem hiding this comment.
SetResource now calls PercentDecode, which is a non-inline symbol defined in the resource library, so configuration_core has a direct dependency on //sdk/src/resource for the first time.
There was a problem hiding this comment.
configuration_core has a dependency on sdk/src/resource due to ConfiguredSdk and SdkBuilder instantiating a resource object. The dependency is being fulfilled transitively from the SDK signal libraries (trace, metrics, and logs) currently. Setting an explicit dependency on resource is better.
| @@ -2552,13 +2554,12 @@ void SdkBuilder::SetResource( | |||
| { | |||
| if (attribute_valid) | |||
There was a problem hiding this comment.
Same as #4460 (comment), attribute_valid is still reachable, so removing the else drops a real diagnostic.
| namespace detail | ||
| { | ||
|
|
||
| std::string PercentDecode(const std::string &value); |
There was a problem hiding this comment.
Could you restore the contract on the declaration as comment?
| #include "opentelemetry/sdk/resource/resource_detector.h" | ||
| #include "opentelemetry/nostd/variant.h" | ||
|
|
||
| #include "detail/percent_decode.h" |
There was a problem hiding this comment.
Same class as the other note on sdk_builder.cc. This is a subdirectory-relative include that bypasses the target's include_prefix.
3 participants
This updates resource.attributes_list handling to percent-decode values before storing them.
Changes include:
Fixes #4456