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 buffer instead string for traceId and spanId #698

Closed
1 of 2 tasks
Flarna opened this issue Jan 15, 2020 · 4 comments
Closed
1 of 2 tasks

Use buffer instead string for traceId and spanId #698

Flarna opened this issue Jan 15, 2020 · 4 comments
Labels
Discussion Issue or PR that needs/is extended discussion.

Comments

@Flarna
Copy link
Member

Flarna commented Jan 15, 2020

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first

The fields traceId and spanId in SpanContext are of type string formatted as hex. This requires several conversions:

  • a buffer to string conversion during creation
  • a string to buffer conversion during exporting (at least if opentelemetry protocol is used)
  • a string to buffer conversion during propagation in binary format

I think it would be more efficient to avoid most of these conversions by keeping the buffer and convert to string only for logging/propagation as string.

Or is there a special reason for using a string?

Another option would be to hide this at all in a dedicated TraceId/SpanId class like e.g. in Java which would allow to hide the actual creation/storage at all and enable caching of different representations.

@vmarchaud vmarchaud added the Discussion Issue or PR that needs/is extended discussion. label Jan 15, 2020
@vmarchaud
Copy link
Member

I'nm not in favor of adding a dedicated class/interfaces for that because i feel it had much complexity that isnt needed there, i would prefer a consensus on one way to create/store those values. Buffer are fine for me.

@draffensperger
Copy link
Contributor

If we go this route, we should use ArrayBuffer for browser compatibility, and it does have decent browser support (see MDN ArrayBuffer docs).

Have you thought about the need to serialize / deserialize these in trace headers for HTTP? I guess I'm not totally convinced that all use cases would benefit from our in-memory representation being an ArrayBuffer.

@Flarna
Copy link
Member Author

Flarna commented Jan 15, 2020

Serialization of trace headers happens only on process entry/exit. Some protcols may need the buffer representation not string like HTTP - maybe to get a more compact/efficient result.

Each process entry/exit is also represented by a Span which usually needs to be exported.
Therefore for HTTP spans with a TraceTag string and binary representation is needed always.
But for internal spans or spans without tags (e.g. database) we need string for in memory representation and binary for export.

@Flarna
Copy link
Member Author

Flarna commented Feb 28, 2020

see #786

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants