4.x Add baggage API and allow access to baggage via SpanContext (as well as Span)
#8320
Conversation
oracle-contributor-agreement
bot
added
the
OCA Verified
tjquinno
requested review from
tomas-langer,
ljnelson,
spericas,
arjav-desai,
klustria and
barchetta
tjquinno
force-pushed
the
4.x-baggage-api
branch
from
b0a05c1 to
ca0db45
Compare
|
|
||
| @Override | ||
| public Set<String> keys() { | ||
| return Collections.unmodifiableSet(values.keySet()); |
There was a problem hiding this comment.
Depending on the frequency of the call, consider storing an unmodifiable Set as an instance variable and returning it directly.
There was a problem hiding this comment.
The values map can grow at any time, so the key set will not be stable. A Set instance field would need to be updated whenever baggage was added or a dirty bit set and the Set reconstituted lazily if dirty upon access. My gut feel is that keys will be infrequently invoked, at least seldom enough for optimizing this to be premature given the map's changeability.
There was a problem hiding this comment.
Fair. (Do note that Collections#unmodifiable... methods account for changing values "underneath"; that's why they're called unmodifiable and not, say, immutable.)
There was a problem hiding this comment.
Exactly. We want the caller to see the accurate key set without being able to mess with its contents.
|
|
||
| @Override | ||
| public Set<String> keys() { | ||
| return Collections.unmodifiableSet(baggage.keySet()); |
| @@ -106,6 +106,9 @@ public <T> T unwrap(Class<T> tracerClass) { | |||
| if (tracerClass.isAssignableFrom(delegate.getClass())) { | |||
| return tracerClass.cast(delegate); | |||
| } | |||
| if (tracerClass.isInstance(this)) { | |||
There was a problem hiding this comment.
Minor nit: depending on how "deep" you want/need the unwrapping to go you could do this whole general thing in a while loop
There was a problem hiding this comment.
The pattern in tracing previously established has been to check only the delegate and this against the requested type and return whichever one matches (and reject the request if neither matches). There's no real "depth" here to be concerned with except for the one level to the delegate.
| } | ||
|
|
||
| @Override | ||
| public WritableBaggage set(String key, String value) { |
There was a problem hiding this comment.
(Naïve question: must it be a WriteableBaggage implementation if it's just going to ignore this call anyway?)
There was a problem hiding this comment.
In this NoOpTracer the inner classes Span and SpanContext have a baggage() method declared by the interfaces they implement to return a WriteableBaggage. Each of the baggage() methods returns theEMPTY_BAGGAGE constant so it needs to implement WriteableBaggage even though, as you point out, it doesn't really write anything.
… well as `Span`) (helidon-io#8320) * Rebase with concurrent changes * Remove an unneeded class * Copyrights
… well as `Span`) (helidon-io#8320) * Rebase with concurrent changes * Remove an unneeded class * Copyrights
Description
Resolves #8319
Users have requested a way to retrieve baggage from a
SpanContextdirectly rather than having to use theSpanContextto create (and start and end) aSpan. Currently only theSpantype allows access to baggage.New types and implementations:
Baggageinterface provides read-only access to baggage withget,keys, andcontainsKeyoperations.WritableBaggageinterface extendsBaggageand adds asetmethod.tracing/tracing) add implementations of these interfaces and add tests.Changes to existing code:
SpanContextinterface adds theBaggage baggage()method.Spaninterface adds theWritableBaggage baggage()method.Spaninterface methods for directly manipulating baggage are marked as deprecated to encourage users to use the more powerfulBaggageinterfaces.Baggageis read-only, typically based on the baggage data from an underlyingSpanContextbecause we don't necessarily know how aSpanContextis derived--for example, it can be from incoming request headers--and so we should not allow callers to update baggage derived from span contexts.In the Helidon neutral tracing API, though, the
Spanallows updates to the baggage we associate with the span, hence theWritableBaggagetype.Very brief guided tour of the changes
For other reasons we already had in the OpenTelemetry provider the
MutableOpenTelemetryBaggageclass which implements an Otel baggage-related interface. This PR expands that class to also implement the new HelidonWritableBaggageinterface.The new
OpenTracingBaggageclass implements the read-onlyBaggageinterface and it has a small inner subclass which implementsWritableBaggage. We use an inner class here because in OpenTracing when span-related baggage is updated we need to propagate the update to the OpenTracing span itself as well as keeping track of it ourselves.Both providers add tests to exercise their implementations of the new baggage API.
The OpenTracing provider component
pom.xmlnow includes several Otel dependencies in test scope. This allows the tests to rely on the actual Otel extractor and propagator, for example, rather than no-ops that would cause tests to fail.Documentation
The PR includes a brief addition to the tracing doc page discussing baggage with links to the JavaDoc.