-
Notifications
You must be signed in to change notification settings - Fork 887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not write a value in Context upon failed extraction. #671
Do not write a value in Context upon failed extraction. #671
Conversation
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
@@ -105,8 +105,8 @@ The implemenation SHOULD preserve casing (e.g. it should not transform `Content- | |||
Extracts the value from an incoming request. For example, from the headers of an HTTP request. | |||
|
|||
If a value can not be parsed from the carrier for a cross-cutting concern, | |||
the implementation MUST NOT throw an exception. It MUST store a value in the `Context` | |||
that the implementation can recognize as a null or empty value. | |||
the implementation MUST NOT throw an exception and MUST NOT store a new value in the `Context`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, it is impossible to distinguish "error" from "no context at all". I could imagine one could use that info to e.g. stop trying more propagators if a propagator found its headers but they were invalid. I think there should be some way for propagators to indicate this. E.g. add a second return value (this is the spec, so it's up to languages how to implement this second return value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A second value is something I have been thinking about, but so far I haven't been able to come up with a clean, simple approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest:
Return values:
- A new
Context
derived from theContext
passed as argument,
containing the extracted value, which can be aSpanContext
,
CorrelationContext
or another cross-cutting concern context. [as before]- A status indicator object. It must be possible to programatically detect at least: "context successfully extracted" and "no context found" and to distinguish them from any other errors the propagator encountered (e.g. header was present but value was invalid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some languages might implement this with an out
parameter, or a class containing both values, whatever is more idiomatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #684 to follow up this possible second value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Oberon00 Would you mind putting your suggestions in the related issue? So we don't forget ;)
This is partially related to #496 as how to proceed when an extraction failed - currently we set a
null
/empty
value, but it can be very useful to retain any previously existing value, as it would help with:Context
allocations for failed extractions.StackPropagator
(which handles multiple signal formats, such as B3 + W3C), as the prototype in Composite propagators: Clarify the behavior of multiple extractor #496 shows.On the other hand, putting a
null
/empty
value inContext
upon extraction clears any previously leaked value, but that's an error in itself anyway.cc @dyladan @Oberon00