make include/exclude easier to use with empty but not null arguments#8185
make include/exclude easier to use with empty but not null arguments#8185SylvainJuge wants to merge 12 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8185 +/- ##
============================================
+ Coverage 90.31% 90.32% +0.01%
- Complexity 7652 7656 +4
============================================
Files 843 843
Lines 23071 23071
Branches 2311 2308 -3
============================================
+ Hits 20836 20840 +4
+ Misses 1516 1515 -1
+ Partials 719 716 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nto fix-another-include-none-by-default
| @@ -74,6 +74,11 @@ public ViewBuilder setAggregation(Aggregation aggregation) { | |||
| */ | |||
| public ViewBuilder setAttributeFilter(Set<String> keysToRetain) { | |||
There was a problem hiding this comment.
[for reviewer] this is the only place where we use IncludeExcludePredicate to potentially remove all items, dealing with this as an exception allows to make most common use-case simpler to call without having to convert empty collection to null.
SylvainJuge
changed the title
sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/IncludeExcludePredicate.java
Outdated
Show resolved
Hide resolved
…nto fix-another-include-none-by-default
…nto fix-another-include-none-by-default
| * <p>When {@code included} is empty or {@literal null}, all values are included by default. | ||
| * | ||
| * <p>When {@code excluded} is empty or {@literal null}, no value is excluded by default. |
There was a problem hiding this comment.
#nit
| * <p>When {@code included} is empty or {@literal null}, all values are included by default. | |
| * | |
| * <p>When {@code excluded} is empty or {@literal null}, no value is excluded by default. | |
| * <p>When {@code included} is empty or {@literal null}, all values are included. | |
| * | |
| * <p>When {@code excluded} is empty or {@literal null}, no value are excluded. |
Applies below as well.
| if (included != null || excluded != null) { | ||
| builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded)); | ||
| } | ||
| builder.setAttributeFilter(IncludeExcludePredicate.createExactMatching(included, excluded)); |
| this.included = included == null ? null : new LinkedHashSet<>(included); | ||
| this.excluded = excluded == null ? null : new LinkedHashSet<>(excluded); | ||
| this.included = copyIfNotEmpty(included); | ||
| this.excluded = copyIfNotEmpty(excluded); |
There was a problem hiding this comment.
Here you're equating empty as the same as null. So empty = include all.
In declarative config, this is impossible because IncludeExclude specified minItems: 1. I.e. its either null or present with at least one item. In practice, we don't validate this in the declarative config factory classes. Should probably extract out a dedicated IncludeExcludeFactory to convert IncludeExcludeModel to Predicate so we can consistently enforce size > 0.
| if (keysToRetain.isEmpty()) { | ||
| // include/exclude predicate requires to include or exclude at least one, we have to skip it | ||
| // when we don't want to include any key. | ||
| return setAttributeFilter(s -> false); |
There was a problem hiding this comment.
We need to translate this to a predicate that has a toString implementation.
3 participants
Fixes a few other occurrences of the same bug fixed in #8177.
When using declarative configuration, we get empty collections when an attribute is not defined, and not
null, when theincludestatement is omitted this will include none and thus exclude all.I have found a few other usages of
IncludeExcludePredicatethat have the same bug as what was fixed in #8177, so I think it is better to provide a more generic solution, for example visible in this intermediate commit : c0e641d (this has now been replaced by the generic solution).This change makes
IncludeExcludePatternrequire that at least one of include and exclude arguments is non-null or empty, in other words we need to have at least one exclusion or one inclusion.