Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-29 18:51 UTC Respond to 8 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8947 +/- ##
============================================
+ Coverage 90.41% 90.45% +0.03%
- Complexity 10981 11033 +52
============================================
Files 1019 1021 +2
Lines 30016 30160 +144
Branches 3917 3937 +20
============================================
+ Hits 27140 27282 +142
+ Misses 1923 1915 -8
- Partials 953 963 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
403a57c to
7d8f465
Compare
3b2ce44 to
e7872d7
Compare
e7872d7 to
657d862
Compare
Implements prototype for Context-scoped attributes per OTEP open-telemetry#4931 and Issue open-telemetry#8532: - Add ContextAttributes API in :api:incubator for setting/getting/extracting attributes on Context - Support opt-in context-scoped attributes configuration across SdkTracerProvider, SdkLoggerProvider, and SdkMeterProvider - Inherit context attributes in SdkSpanBuilder, SdkLogRecordBuilder, and synchronous metric storage (Delta & Cumulative) - Expose configuration via declarative-config (tracer_provider, logger_provider, meter_provider) - Add comprehensive unit, incubating, and multi-signal integration tests
657d862 to
ef1468f
Compare
|
@jack-berg Please take a look . Thank you. |
| if (context == null) { | ||
| return Attributes.empty(); | ||
| } | ||
| Attributes attributes = context.get(CONTEXT_ATTRIBUTES_KEY); |
There was a problem hiding this comment.
I see usefulness in this feature but I find it hard to understand how to decide with instrumentations should include the attributes.
How to assert or qualify the carnality of the attribute?
Which instrumentations should skip an attribute?
Should we have a handler to decide which instrumentations opt to an attribute?
Are there attributes that make no sense across an entire context lifespan?
jack-berg
left a comment
jack-berg
left a comment
There was a problem hiding this comment.
Pretty good stuff - close to what I expected. Left a few comments but nothing should be too tricky.
| private LoggerProviderModelAccessor() {} | ||
|
|
||
| static final String LOGGER_CONFIGURATOR = "logger_configurator/development"; | ||
| static final String CONTEXT_SCOPED_ATTRIBUTES = "context_scoped_attributes/development"; |
There was a problem hiding this comment.
These are generated from the JSON schema defined in opentelemetry-configuration. We'll have to skip declarative config support until someone goes and defines the config surface area in the schema.
| * Utility for associating {@link Attributes} with a {@link Context} for automatic propagation to | ||
| * telemetry signals produced within that context. | ||
| */ | ||
| public final class ContextAttributes { |
There was a problem hiding this comment.
The otep was merged for this, but there has been no attempt yet to codify the otep in a spec PR. This repo is bound to content in the spec (otep != spec). I'm going to make a judgement call here and go ahead with this on the basis of the otep, but it can't live on indefinitely if nobody makes progress in the spec.
I want there to be a comment here reminding us of this.
| public final class ContextAttributes { | |
| // TODO (jack-berg): delete after 2027-07-01 if progress hasn't been made on https://github.com/open-telemetry/opentelemetry-java/issues/8532 | |
| public final class ContextAttributes { |
| * SdkLoggerProviderUtil#setContextScopedAttributesEnabled(SdkLoggerProviderBuilder, boolean)}. | ||
| */ | ||
| SdkLoggerProviderBuilder setContextScopedAttributesEnabled( | ||
| boolean contextScopedAttributesEnabled) { |
There was a problem hiding this comment.
I know the otep talks about configuration as a boolean, but this is not going to be the final form. Its not expressive enough to handle cases where you have high and low cardinality attributes, and you want to include the low cardinality attribute on all signals and the high cardinality attributes only on spans and logs.
I'd like to start with a BiPredicate<InstrumentationScopeInfo, AttributeKey<?>>. This should give you expressiveness where you want / need it, without compromising convenience since you can just configure (unused, unused) -> true just as easily as true, and shouldn't impact performance.
| if (!contextAttributes.isEmpty()) { | ||
| AttributesMap mergedAttributes = | ||
| recordAttributes != null | ||
| ? AttributesMap.copyOf(recordAttributes) |
There was a problem hiding this comment.
Copying the builder's attributes seems like a safety mechanism to prevent context attributes polluting the builder attributes in case of reuse. But builder is already not safe for reuse since attributes are passed to the SdkReadWriteLogRecord where a processor can mutate them and which allows a caller to further mutate the attributes after the fact.
So I think we should take steps to explicitly prevent builder reuse (separately from this PR) but don't need to spend the extra resources making a copy here.
| AttributesMap spanAttributes = attributes(); | ||
| contextAttributes.forEach( | ||
| (key, value) -> { | ||
| if (spanAttributes.get(key) == null) { |
There was a problem hiding this comment.
This check (and the corresponding in logs) checks for collisions using attribute name and type, where as put(AttributeKey<?>, Object) only considers attribute name (correctly since its invalid to have attributes of the same name and different type). Can you make this check consistent?
| @SuppressWarnings({"rawtypes", "ReferenceEquality"}) | ||
| public Span startSpan() { | ||
| Context parentContext = parent == null ? Context.current() : parent; | ||
| if (noParent |
There was a problem hiding this comment.
Should enabling attribute inheritance also change which unrelated context values samplers and processors receive for setNoParent()? Could we source inherited attributes from the ambient context without changing the existing parent-context behavior?
| * and {@code attributes}. If the same attribute key is present in both, the value from {@code | ||
| * attributes} takes precedence. | ||
| */ | ||
| public static Context add(Context context, Attributes attributes) { |
There was a problem hiding this comment.
Should probably make this set instead of add, since set is more flexible: with add you can't get replacement semantics, but with set you can replace or add (via get then set) depending on what you want.
Overview
Prototypes Context-scoped attributes (OTEP #4931, #8532), allowing Traces, Logs, and synchronous Metrics to automatically inherit attributes from the active
Contextwithout manually adding them at every call site.Key Decisions (Spec Gaps Resolved)
bind(), but Java incubator retains it.BoundStorageHandle) record only to their pre-bound timeseries with zero runtime attribute merging. This guarantees zero allocations on hot paths and prevents cardinality explosion on shared infrastructure (e.g., connection pools). Unbound instruments continue to merge context attributes dynamically.setNoParent()preserves Context Attributes:setNoParent()historically reset context toContext.root(), unintentionally deleting context attributes and baggage.Context.root()withcurrent.with(Span.getInvalid()). This detaches the parent span while keeping context attributes and baggage available forSamplerandSpanProcessor. UsesetParent(Context.root())for full isolation.Signal Implementation Summary
api:incubator):ContextAttributes.add(Context, Attributes)(merges into context) andget(Context)(SDK reader).sdk:trace): Merges context attributes beforeSampler.shouldSample(); unifies context across sampling, span creation, andSpanProcessor.onStart().sdk:logs): Merges onemit()into an isolated record attributes map to prevent attribute leakage when reusingLogRecordBuilder.sdk:metrics): Merges context attributes on unbound instruments before view filtering; bypassescanUseRootContextoptimization when active.Sdk*ProviderUtilor declaratively viacontext_scoped_attributes/development.Verification
ContextScopedAttributesSpanTest(attributes, precedence, limits, andsetNoParent()context preservation) &SdkSpanBuilderTest(legacy compatibility).ContextScopedAttributesMetricTest(dynamic unbound enrichment, bound handle isolation, same-context co-existence).ContextScopedAttributesLogTest(merging, collision precedence, builder reuse).ContextScopedAttributesIntegrationTest(multi-signal E2E); clean runs acrossspotless,checkstyle,jApiCmp, andanimalsniffer.