Skip to content
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

Merged

Conversation

jack-berg
Copy link
Member

Adds a helper function for specifying that a view should retain only a specific set of attribute keys:
Previous:

Set<String> keysToRetain = new HashSet<>(Arrays.asList("key1", "key2"));
View view = View.builder().setAttributesFilter(keysToRetain::contains);

After:

View view = View.builder().setAttributesFilter(new HashSet<>(Arrays.asList("key1", "key2")));

As an added benefit, we can have ensure a proper toString() implementation when the helper function is used:

Set<String> keysToRetain = new HashSet<>(Arrays.asList("key1", "key2"));
View view = View.builder().setAttributesFilter(keysToRetain::contains);
view.toString(); // View{aggregation=DefaultAggregation, attributesProcessor=AttributeKeyFilteringProcessor{nameFilter=io.opentelemetry.sdk.metrics.ViewTest$$Lambda$405/0x0000000800263508@1dcca8d3}, cardinalityLimit=2000}

View view = View.builder().setAttributesFilter(new HashSet<>(Arrays.asList("key1", "key2")));
view.toString(); // View{aggregation=DefaultAggregation, attributesProcessor=AttributeKeyFilteringProcessor{nameFilter=SetIncludesPredicate{set=[key1, key2]}}, cardinalityLimit=2000}

@jack-berg jack-berg requested a review from a team August 24, 2023 18:42
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e0a0b77) 91.29% compared to head (93df5ba) 91.30%.
Report is 3 commits behind head on main.

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     
Files Changed Coverage Δ
...java/io/opentelemetry/sdk/metrics/ViewBuilder.java 91.66% <100.00%> (+1.19%) ⬆️
...sdk/metrics/internal/view/AttributesProcessor.java 77.27% <100.00%> (+8.52%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -73,7 +82,8 @@ public ViewBuilder setAggregation(Aggregation aggregation) {
*/
public ViewBuilder setAttributeFilter(Predicate<String> keyFilter) {
Objects.requireNonNull(keyFilter, "keyFilter");
return addAttributesProcessor(AttributesProcessor.filterByKeyName(keyFilter));
Copy link
Member

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

Copy link
Member Author

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}ProviderBuilders, 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.

@jack-berg jack-berg merged commit 2deb6d1 into open-telemetry:main Aug 25, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants