-
Notifications
You must be signed in to change notification settings - Fork 651
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
Adding propagators API and b3 SDK implementation (#51, #52) #78
Adding propagators API and b3 SDK implementation (#51, #52) #78
Conversation
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Show resolved
Hide resolved
A general question - do we expect the API package to provide implementations for:
The reason for this ask is that we have a scenario that users only have API package installed, and the application should work (which means if the application received W3C TraceContext headers, it should follow the W3C spec and process the headers). |
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.
Minor comments, but this LGTM pending the docstring fixes. You may want to include the tracer methods to get the propagators in this PR, or add them in a follow up PR.
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Outdated
Show resolved
Hide resolved
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Show resolved
Hide resolved
That's my understanding, the API should pass the headers through if there's no SDK loaded. It's also what the java client does. I think it's fine to add the implementation (in the API) in another PR since it will just implement these abstract classes, not replace them. |
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Show resolved
Hide resolved
Sounds good. I'm not strongly opinionated on where to place them, although I will note that moving implementations to SDK pretty much guarantees that opentelemetry-sdk will always be consumed, as I imagine most distributed trace implementations will propagate one of binary / w3c tracestate / b3 headers anyway. If we move all propagators to the API then there's a chance nothing from SDK will be necessary (although Resource's implementation is in SDK as well). |
I've submitted / contributed to 3 Issues in the specification around some of the design decisions here: open-telemetry/opentelemetry-specification#112 Guidance from Ted on the gitter is to create a python example of how some of these improvements will work. I'll focus on cleaning up the code in this PR, and add a future PR around how to use these propagators that will answer questions like where the registry lies. https://gitter.im/open-telemetry/opentelemetry-specification?at=5d4f492153490e334dccf927 |
incorporating unit tests and a more lenient implementation of the b3 propagator. Fixing a bug with b3 propagators consuming and producing integers rather than hex-encoded values.
from opentelemetry.trace import SpanContext | ||
|
||
|
||
class BinaryFormat(abc.ABC): |
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 think this class could use some "performance-oriented" APIs: Since creating a slice of bytes
copies it, I suggest from_bytes(byte_representation:bytes, offset: int = 0, length: int = -1)
, and maybe append_bytes(dst: bytearray)
.
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 can see the value here, but I may wait on adding these until I add an implementation (e.g. the standard binary format). That might better illustrate the right API, unless you already have an example in mind.
Alternatively it looks like memoryview would be a great way to read values:
https://docs.python.org/3/library/stdtypes.html#memoryview
Although I could always construct a memoryview from the bytes object I received. For to_bytes I could use bytearray internally to construct the buffer.
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.
In case it's helpful, OC uses memoryview in the tags (read: distributed context) propagator: https://github.com/census-instrumentation/opencensus-python/blob/577fb61ab26513e3b448b6696432eefb0b11fa73/opencensus/tags/propagation/binary_serializer.py#L38.
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Outdated
Show resolved
Hide resolved
opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py
Outdated
Show resolved
Hide resolved
refactoring formatting of trace / spans for b3 as it's used frequently. renaming trace_id / span_id in b3 unit tests for clarity. removing unneeded int casts.
opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py
Show resolved
Hide resolved
I have a general question which more related to the spec. |
from opentelemetry.trace import SpanContext | ||
|
||
|
||
class BinaryFormat(abc.ABC): |
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.
In order to not duplicate the same class for correlation-context (distributedcontext) I would suggest to make this a template and have BinaryFormat<SpanContext>
and BinaryFormat<DistributedContext>
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.
Thanks! Mypy typing does support generics, but I'm planning of filing another PR that aligns closer to the tickets I've mentioned above. This will include creating a unified composed object for SpanContext and DistributedContext and using the composed object as the context for propagators, thereby eliminating the need for the generic.
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.
From an offline conversation with @bogdandrutu: a bigger problem is that referencing SpanContext
here would make context
depend on trace
.
This creates a circular dependency, and even if we remove the propagators from the tracer trace
will depend on context
in other places. If we think of the context
package as representing the base context propagation layer it's surprising to see it depend on a layer higher in the stack.
This isn't a blocking comment for this PR, just something to keep in mind when you start composing context objects.
HTTP Headers can contains multiple values for the same key. This is important to support for formats such as w3c tracestate.
@staticmethod | ||
@abc.abstractmethod | ||
def from_bytes(byte_representation: bytes) -> typing.Optional[SpanContext]: | ||
"""Return a SpanContext that was represented by bytes. |
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.
As we discussed today, we might want to use the same propagator for both SpanContext
and DistributedContext
.
No need to be blocked though, we can address this in another PR.
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.
yes, coming up in the next PR. want to get this merged in first.
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.
Roger that.
must be paired with an appropriate get_from_carrier | ||
which understands how to extract a value from it. | ||
Returns: | ||
A SpanContext with configuration found in the carrier. |
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.
If the carrier doesn't provide a SpanContext, do we expect to generate one or return None?
This could become tricky since we might receive something between valid and invalid. For example
https://github.com/w3c/trace-context/blob/b145878f5618fccbf4926bc181ecb25b709b111d/test/test.py#L536.
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.
According to the the Specification (https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md#frombytes):
If the value could not be parsed, the underlying implementation SHOULD decide to return ether an empty value, an invalid value, or a valid value.
In Java we do not return null
ever for this (makes me wonder if we should tune the Specification for this), and we instead will be returning SpanContext.getInvalid()
;)
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.
The current organization of the API makes the ramifications of this choice and how to handle it a decision of whoever would handle handing the SpanContext back to the context. I.E. error handling and how to deal with that is up to whoever calls the extract / inject method.
I think as a convention the formats should return either a valid value or nothing at all. But honestly I think we'll see changes here as we discover more use cases around things like needing to support multiple propagators.
elif len(fields) == 4: | ||
trace_id, span_id, sampled, _parent_span_id = fields | ||
else: | ||
return trace.INVALID_SPAN_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.
👍
@reyang @c24t @carlosalberto @Oberon00 if all looks good, can I get an approval? Have a proposal PR waiting on merging this one. |
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.
The code is good, there are some open design questions that we need to figure out. We can merge this then explore the open questions in separate PRs.
- Where to put propagator (not likely to be Tracer, very likely to be with integrations)?
- How to decouple formatter and propagator?
- Do we handle SpanContext and DistributedContext in the same propagator (most likely yes!)
- How many DistributedContext do we want to have (e.g. is each Tracer having its own version)?
Yes! Great points. I have a PR that has a proposal for 3. I'll put 1,2 on my list to get proposals for. 4 has far-reaching impacts so I imagine we'll start considering that case after our initial release date. |
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.
Only minor comments, this LGTM pending @reyang and @carlosalberto's approval.
Thanks for taking on the propagators! This PR has unearthed a lot of assumptions about context prop.
from opentelemetry.trace import SpanContext | ||
|
||
|
||
class BinaryFormat(abc.ABC): |
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.
From an offline conversation with @bogdandrutu: a bigger problem is that referencing SpanContext
here would make context
depend on trace
.
This creates a circular dependency, and even if we remove the propagators from the tracer trace
will depend on context
in other places. If we think of the context
package as representing the base context propagation layer it's surprising to see it depend on a layer higher in the stack.
This isn't a blocking comment for this PR, just something to keep in mind when you start composing context objects.
PROPAGATOR = HTTPTextFormat() | ||
|
||
|
||
|
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.
Why all the blank lines?
get_from_carrier: a function that can retrieve zero | ||
or more values from the carrier. In the case that | ||
the value does not exist, return an empty list. | ||
carrier: and object which contains values that are |
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.
carrier: and object which contains values that are | |
carrier: an object which contains values that are |
"""Create a SpanContext from values in the carrier. | ||
|
||
The extract function should retrieve values from the carrier | ||
object using get_from_carrier, and use values to populate a |
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.
object using get_from_carrier, and use values to populate a | |
object using `get_from_carrier`, and use values to populate a |
|
||
The extract function should retrieve values from the carrier | ||
object using get_from_carrier, and use values to populate a | ||
SpanContext value and return it. |
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.
SpanContext value and return it. | |
`SpanContext` value and return it. |
And the same changes to other docstrings. This is to make the sphinx docs render nicely, but feel free to omit the sphinx markup where it hurts code readability.
I haven't been including sphinx markup in the first line of the docstrings since we need them to fit on a single line, but since we changed the sphinx default role most markdown is just backticks now, and it may be worth the extra characters to do this consistently.
Specification: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md.