-
Notifications
You must be signed in to change notification settings - Fork 834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AttributesProcessor toString, add attribute filter helper #5765
Add AttributesProcessor toString, add attribute filter helper #5765
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5765 +/- ##
=========================================
Coverage 91.29% 91.30%
- Complexity 5245 5256 +11
=========================================
Files 585 586 +1
Lines 15537 15616 +79
Branches 1502 1518 +16
=========================================
+ Hits 14185 14258 +73
- Misses 930 931 +1
- Partials 422 427 +5
☔ View full report in Codecov by Sentry. |
@@ -73,7 +82,8 @@ public ViewBuilder setAggregation(Aggregation aggregation) { | |||
*/ | |||
public ViewBuilder setAttributeFilter(Predicate<String> keyFilter) { | |||
Objects.requireNonNull(keyFilter, "keyFilter"); | |||
return addAttributesProcessor(AttributesProcessor.filterByKeyName(keyFilter)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the addAttributesProcessor
method be removed? It does not seem to be used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets called reflectively by SdkMeterProviderUtil
, which provides experimental methods for adding attributes processors which add keys from baggage.
The change here is worth calling out. I've changed the behavior of setAttributeFilter
from chaining together to setting the attribute filter, erasing any previous. I believe this is the correct, and is consistent with out places where we have the opportunity to merge or override in a builder. For example, in the Sdk{Signal}ProviderBuilder
s, we have setResource
, which sets the resource, overriding any previously set, and addResource
, which merges with the existing.
If generic attribute filters become part of our public API some day, we can consider having both set*
and add*
variants.
Adds a helper function for specifying that a view should retain only a specific set of attribute keys:
Previous:
After:
As an added benefit, we can have ensure a proper
toString()
implementation when the helper function is used: