Repository navigation
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-11-09 20:12 UTC Two things need attention:
Status above doesn't look right?
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8947 +/- ##
============================================
- Coverage 90.41% 90.35% -0.07%
- Complexity 10981 11085 +104
============================================
Files 1019 1023 +4
Lines 30016 30306 +290
Branches 3917 3953 +36
============================================
+ Hits 27140 27383 +243
- Misses 1923 1956 +33
- Partials 953 967 +14 ☔ 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?
There was a problem hiding this comment.
Instrumentations are advised against including attributes (see javadoc at top).
Think about the core use cases: tenant id / user id. An application would add these to context early in request lifecycle via interceptor or filter. They would also configure the SDK to include the attributes as needed based on signal, scope, and and attribute key.
The OTEP left a lot of questions unanswered with the intent of discovering more via prototyping. If you have general feedback (i.e. not related to the java implementation), please add it to open-telemetry/opentelemetry-specification#5170 so all languages can benefit.
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.
| @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?
|
Hi @rooks-ns — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
…oped attributes prototype (open-telemetry#8532) - Trace: Isolate parent context and ambient Baggage when setNoParent() is configured by passing Context.root() to samplers/processors while resolving context attributes from current context. - Collisions: Add amortized O(1) AttributesMap.contains(AttributeKey<?>) to enforce name-based attribute identity and ensure explicit attributes take precedence over context attributes regardless of type. - Logs: Eliminate AttributesMap.copyOf defensive allocation by mutating builder attributes in-place during emit(). - Filtering: Upgrade trace, log, and metric providers from a boolean flag to BiPredicate<InstrumentationScopeInfo, AttributeKey<?>> contextScopedAttributesFilter; curry predicate in SdkMeter for synchronous metric storage; preserve context-oblivious bound instruments. - API: Add ContextAttributes.set() for replacement and clearing, retain add() for additive merging, enforce strict requireNonNull validation, and record maintainer deprecation TODO. - Config: Revert declarative-config prototype additions to match upstream main.
12e5e76 to
a9bcfb9
Compare
| import javax.annotation.Nullable; | ||
|
|
||
| // TODO (jack-berg): delete after 2027-07-01 if progress hasn't been made on | ||
| // https://github.com/open-telemetry/opentelemetry-java/issues/8532 |
There was a problem hiding this comment.
| // https://github.com/open-telemetry/opentelemetry-java/issues/8532 | |
| // https://github.com/open-telemetry/opentelemetry-specification/pull/4931 |
Its progress on the spec we want to gate on
| * Utility for associating {@link Attributes} with a {@link Context} for automatic propagation to | ||
| * telemetry signals produced within that context. | ||
| * | ||
| * <h2>Usage and Authoring Guidance</h2> |
There was a problem hiding this comment.
Less is more here.
General library instrumentations should avoid populating context-scoped attributes to prevent injecting library-internal concerns into ambient application telemetry.
If this isn't a direct quote from the otep, omit it.
Context-scoped attributes are stamped across all active telemetry signals produced within the context.
Not by default. Should more simply indicate that SDKs can configure context scoped attributes to be stamped across telemetry signals, with the ability to configure based on signal, scope, and attribute key. The metric cardinality warning is good to keep.
| */ | ||
| public static Context add(Context context, Attributes attributes) { | ||
| requireNonNull(context, "context"); | ||
| requireNonNull(attributes, "attributes"); |
There was a problem hiding this comment.
Should log and gracefully degrade if these are null, following advice in "docs/knowledge/api-design.md -> Null guards -> Runtime / instrumentation-time boundaries (Span methods, metric recordings, log builders)"
| if (context == null) { | ||
| return Attributes.empty(); | ||
| } | ||
| Attributes attributes = context.get(CONTEXT_ATTRIBUTES_KEY); |
There was a problem hiding this comment.
Instrumentations are advised against including attributes (see javadoc at top).
Think about the core use cases: tenant id / user id. An application would add these to context early in request lifecycle via interceptor or filter. They would also configure the SDK to include the attributes as needed based on signal, scope, and and attribute key.
The OTEP left a lot of questions unanswered with the intent of discovering more via prototyping. If you have general feedback (i.e. not related to the java implementation), please add it to open-telemetry/opentelemetry-specification#5170 so all languages can benefit.
| * | ||
| * @param key the attribute key whose name to look up. Must not be null. | ||
| */ | ||
| public boolean contains(AttributeKey<?> key) { |
There was a problem hiding this comment.
If its checking containment regardless of type, let's have the method signature reflect this by accepting String. The current signature suggests this method is redundant to calling get(AttributeKey) != null.
| * the series is resolved at bind time. Backs bound instruments. | ||
| * the series is resolved at bind time. Backs bound instruments. Bound handles record strictly | ||
| * against their bound attributes and do not inherit dynamic context-scoped attributes, preserving | ||
| * bare-metal performance and isolating infrastructure metrics from runtime cardinality explosion. |
There was a problem hiding this comment.
Bare metal isn't quite right. Also, "isolating infrastructure metrics" suggests that bound instruments are somehow related to infrastructure metrics, but bound instruments are a good choice whenever you know the label set up front.
I think better to just say that bound handles do not inherit dynamic context-scoped attributes, and maybe add a TODO to review this.
| this.memoryMode = memoryMode; | ||
| } | ||
|
|
||
| CumulativeSynchronousMetricStorage( |
There was a problem hiding this comment.
No new overloads for internal classes like this. Just update the existing constructor to include the new param and update all the tests which reference it accordingly. Same applies elsewhere.
| @SuppressWarnings({"rawtypes", "unchecked"}) | ||
| private Attributes getMergedContextAttributes(Attributes attributes, Context context) { | ||
| if (contextScopedAttributesFilter == null) { | ||
| return null; |
There was a problem hiding this comment.
Why return null instead of just returning the attributes param unchanged?
| return null; | ||
| } | ||
| boolean[] hasMatched = new boolean[1]; | ||
| AttributesBuilder builder = Attributes.builder(); |
There was a problem hiding this comment.
Can we avoid initializing the builder until we know we have a matched attribute?
| if (contextScopedAttributesFilter == null) { | ||
| return null; | ||
| } | ||
| Attributes contextAttributes = StateIncubatingUtil.getContextAttributes(context); |
There was a problem hiding this comment.
I don't see a guard that ensures we don't call incubating utils unless we know that incuabtor is on the classpath. I think the test is passing because the check on line 164 essentially gates the same thing. But what if someone configures a contextScopedAttributesFilter without the api-incubator present?
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.