Skip to content
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

Use hex encoding for trace id and span id fields in OTLP JSON encoding #911

Conversation

tigrannajaryan
Copy link
Member

Resolves: #786

See discussion and motivation for the change in the issue linked above.

@tigrannajaryan tigrannajaryan requested review from a team as code owners September 1, 2020 20:28
@tigrannajaryan tigrannajaryan added the spec:protocol Related to the specification/protocol directory label Sep 1, 2020
@tigrannajaryan tigrannajaryan added the spec:trace Related to the specification/trace directory label Sep 1, 2020
Resolves: open-telemetry#786

See discussion and motivation for the change in the issue linked above.
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/otlpjson-base64-to-hex branch from 3ba61f1 to 93d0a6d Compare September 1, 2020 20:29
@bogdandrutu
Copy link
Member

Is it a simple way to still use the protobuf encoder with this? Asking for a friend :)))

@tigrannajaryan
Copy link
Member Author

I only tried with Gogoproto, and it was not trivial, required a custom marshaler and custom data type for the fields. Here is the draft code on my branch: open-telemetry/opentelemetry-collector@master...tigrannajaryan:feature/tigran/hexid

I am not sure if this is doable with official protobuf libraries and how easy it is.

@bogdandrutu
Copy link
Member

@tigrannajaryan does it worth the effort then?

@tigrannajaryan
Copy link
Member Author

@tigrannajaryan does it worth the effort then?

I think it is worth it, see some thoughts on why here #786 (comment)

Some of the work that is needed to make this work with Gogoproto we wanted to do anyway so that we can reduce the number of allocations for traceid/spanid slices in the Collector by using custom data types. Once it is done there is only some additional work needed to customize the JSON representation.

For some language SDKs this is trivial if I understand correctly (e.g. for JS) since they don't use protobuf JSON encoding capabilities but encode manually instead and it is a one-line change for them.

Even without Protobuf libraries it is not a big stretch to just code the entire JSON marshaling/unmarshaling manually using whatever JSON lib is available. Tests will probably take most of the time if one wants to do this.

@SergeyKanzhelev
Copy link
Member

It would be super nice to have identifiers as hex. It will simplify life for humans.

Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM given @tigrannajaryan knows how to implement it. Should we file a feature request at https://github.com/protocolbuffers/protobuf/issues?

@codeboten
Copy link
Contributor

codeboten commented Sep 2, 2020

@tigrannajaryan am I reading this change linked in the comments above correctly, 6000+ files changed with 1.9 million lines of code?

@tigrannajaryan
Copy link
Member Author

@tigrannajaryan am I reading this change linked in the comments above correctly, 6000+ files changed with 1.9 million lines of code?

Ignore the vendor directory. Vendoring the dependencies was just the quickest way to try it.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:protocol Related to the specification/protocol directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trace/Span ids base64 encoded when used in proto-JSON representation
6 participants