Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
OT Collector trace exporter #405
OT Collector trace exporter #405
Changes from 12 commits
d6aa116
83267f6
1c79ae6
444334c
2bab1cc
bd7a021
e36cf3e
809a27c
3c769e1
f90526d
242704a
3c794df
3a66e47
744b372
402e6d6
4c48178
a4d93b9
9cac0c2
bb43c70
2c9c018
dc1274e
8f5bbe9
f7feccb
64250e0
3df18a4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
should this have a protocol prefixing the host name?
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.
Updated, is actually expecting machine/container name
https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/agent/common/v1/common.proto#L58
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.
This is an OpenCensus exporter? You write OT exporter in the title.
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.
Well is in fact exporting to OT Collector through OpenCensus receiver, OT receiver will be ready in several weeks I added more details in first comment in the PR, we will need to revisit this one and add code to handle OT receiver using OT proto
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.
FYI I spent some time trying to make it work with OT proto building the protobuf files myself then realizing the receiver is not there yet, people are interested in having this ready so decided to take same approach as JS SDK and support it through OpenCensus receiver, once OT receiver is ready hopefully changes only affect the span transformation and some other small pieces of code
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.
Out of curiosity: Why "big"?
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.
There is no particular reason, is there any issue with 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.
I want to know if the collector expects the traces in a particular format, it is possible that a collection system is not able to assemble a full trace if we use the wrong trace id here.
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.
https://github.com/census-instrumentation/opencensus-proto/blob/master/src/opencensus/proto/trace/v1/trace.proto#L41 there are no details about that, @owais s this something you know?
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.
It shouldn't matter as long as the trace ID is consistent in all spans for a trace but I still suggest to test it out end to end. Generate a trace in python, export using this lib and check how the collector interprets it. You can use the file exporter to write the received spans to a file
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.
This absolutely matters. Traces can span multiple systems, multiple OpenTelemetry implementations/languages, even systems not using OpenTelemetry at all but e.g. OpenTracing+jaeger to report to the same back end.
The "problem" here is that we use integers in Python to represent the trace ID, which is semantically a byte array. If we get an incoming trace ID like "4bf92f3577b34da6a3ce929d0e0e4736" then I think it is clear that
trace_id[0] == 0x4b
andtrace_id[15] == 0x36
must be the case. Thus, "big" sounds correct.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 configured an http server example to use the collector exporter and a client to use Jaeger, when "big" is used the trace is correctly assembled, so "big" should be the right choice.
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.
However I'm not sure this is true in all cases, so probably we want to keep an eye on this int <-> bytes <-> string conversions.
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.
When we read the trace ID from the wire we assume big-endianness, so "big" is right here unless we want to reverse it on the way out.
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 is not possible.
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 believed I triggered this one with a manually created Span in a unit test, same as others checks in this method I added if x instead of if x is not None.
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.
This cannot happen:
opentelemetry-python/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Line 173 in 72862c9
Maybe you wan to use
if span.attributes
instead, but I guess the check can simply be omitted.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 have multiple "if" checks without the "is not None" for all collections now, I guess is safer to check even if we expect this value to be there.