-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
perf: speed up otel trace generation #26854
base: main
Are you sure you want to change the base?
Conversation
@@ -192,13 +223,28 @@ class Span { | |||
this.enter(); | |||
} | |||
|
|||
get traceId() { |
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.
these properties don't exist in otel. if buffers are fastest we could just do this.traceId = buffer
without using a private. if/when we do perf work in upstream otel, we could do similar, with strings only being created in places where the api expects 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.
We could also remove these fields entirely then and only have the spanContext()
API?
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, the original idea was to avoid needing to use spanContext
, since the extra object allocation is theoretically unneeded when flushing a span.
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.
Yeah, but if you look at how Deno.tracing.Span
spans are flushed now, they don't access either spanContext
or traceId
(only the internal #traceId
).
Speeds up trace generation by about 30%.