Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-27 15:42 UTC Move out of draft to request review. Status above doesn't look right?
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8979 +/- ##
============================================
- Coverage 90.35% 90.35% -0.01%
- Complexity 10984 11006 +22
============================================
Files 1019 1019
Lines 30072 30148 +76
Branches 3927 3937 +10
============================================
+ Hits 27173 27241 +68
- Misses 1939 1943 +4
- Partials 960 964 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
psx95
left a comment
psx95
left a comment
There was a problem hiding this comment.
When registering the associated storage for metrics in SdkMeter, if a view has drop aggregation, then the storage for that is skipped. This could probably cause an issue in the intended functionality because opentelemetry-java evaluates each matching RegisteredView independently in SdkMeter and simply skips (continue) any view whose aggregation is Aggregation.drop(), registering both:
- InstrumentSelector("*") →
enabled = true - InstrumentSelector("my.unwanted.instrument") →
enabled = falseand this will not disable "my.unwanted.instrument" since the second view turns intoAggregation.drop()and is skipped by SdkMeter, while the first view (*) still registers a live MetricStorage for "my.unwanted.instrument".
IIUC, this is a key use-case of the spec, right?
| UnaryOperator<Attributes> attributesFilter, | ||
| int cardinalityLimit, | ||
| SourceInfo viewSourceInfo, | ||
| @Nullable Boolean enabled) { |
There was a problem hiding this comment.
Why is this Nullable? We already have a constructor which can be used when the user does not wish to explicitly provide enabled.
| assertThat(counter2.isEnabled()).isFalse(); | ||
|
|
||
| counter1.add(1); | ||
| counter2.add(1); |
There was a problem hiding this comment.
nit: Perhaps adding an assertion to reflect the expected behavior of updating a disabled counter might be good.
|
|
||
| private static RegisteredView applyAdviceToDefaultView( | ||
| RegisteredView instrumentDefaultView, Advice advice) { | ||
| RegisteredView registeredView, Advice advice) { |
There was a problem hiding this comment.
Just curious: Why the name change?
| if (matchesSelector(entry.getInstrumentSelector(), descriptor, meterScope)) { | ||
| if (Boolean.FALSE.equals(entry.getEnabled())) { | ||
| result.add(applyDropAggregation(entry)); | ||
| continue; |
There was a problem hiding this comment.
This would force the next iteration without checking isCompatibleWithInstrument verification.
| if (descriptor.getAdvice().isOptIn() && !Boolean.TRUE.equals(entry.getEnabled())) { | ||
| result.add(applyDropAggregation(entry)); | ||
| } else if (Boolean.TRUE.equals(entry.getEnabled()) | ||
| && descriptor.getAdvice().hasAttributes() | ||
| && entry.getView().getAttributeFilter().equals(StringPredicates.ALL)) { | ||
| result.add(applyAdviceToDefaultView(entry, descriptor.getAdvice())); | ||
| } else { | ||
| result.add(entry); |
There was a problem hiding this comment.
Can we add minor comments on these if-else conditions to clarify the intent here?
Currently, it seems like on a standard instrument (OptIn = false) with AttributesAdvice would be treated differently depending on enabled being unset or true.
Setting enabled = true on a standard instrument would cause the advice to be applied, but leaving enabled unset would not. But it looks like this is a side effect of the current implementation rather than intention.
Summary
Implements the experimental
OptIninstrument advisory parameter andViewenabledstream configuration defined in open-telemetry/opentelemetry-specification#4809:opentelemetry-api-incubator):setOptInAdvice(boolean)(default no-op) to all 8Extended*Builderinterfaces (ExtendedDoubleCounterBuilder,ExtendedDoubleGaugeBuilder,ExtendedDoubleHistogramBuilder,ExtendedDoubleUpDownCounterBuilder,ExtendedLongCounterBuilder,ExtendedLongGaugeBuilder,ExtendedLongHistogramBuilder,ExtendedLongUpDownCounterBuilder).opentelemetry-sdk-metrics):isOptIn()/setOptIn(boolean)to internalAdvice/AdviceBuilderand wiressetOptInAdvice(boolean)across allExtendedSdk*Builderimplementations (preservingoptInacross.ofDoubles()/.ofLongs()builder swaps).ViewBuilder#setEnabled(boolean)andView#getEnabled(), exposed experimentally viaSdkMeterProviderUtil#setEnabled(ViewBuilder, boolean).ViewRegistry#findViewsso that:OptIn=trueuseAggregation.drop()by default when no view matches or when a matching view does not setenabled=true.enabled=falseuseAggregation.drop()regardless of the view's configured aggregation.enabled=trueenableOptIn=trueinstruments while still respecting other instrument advisory parameters (AttributesandExplicitBucketBoundaries) unless overridden by the view.