Add SpanProfiler #448
Merged
Add SpanProfiler #448
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kolesnikovae
commented
Dec 8, 2023
Comment on lines
+83
to
+109
| // isRemoteSpan reports whether the span context represents a remote parent. | ||
| // | ||
| // NOTE(kolesnikovae): this is ugly, but the only reliable method I found. | ||
| // The opentracing-go package and Jaeger client are not meant to change as | ||
| // both are deprecated. | ||
| func isRemoteSpan(c jaeger.SpanContext) bool { | ||
| jaegerCtx := *(*jaegerSpanCtx)(unsafe.Pointer(&c)) | ||
| return jaegerCtx.remote | ||
| } | ||
|
|
||
| // jaegerSpanCtx represents memory layout of the jaeger.SpanContext type. | ||
| type jaegerSpanCtx struct { | ||
| traceID [16]byte // TraceID | ||
| spanID [8]byte // SpanID | ||
| parentID [8]byte // SpanID | ||
| baggage uintptr // map[string]string | ||
| debugID [2]uintptr // string | ||
|
|
||
| // samplingState is a pointer to a struct that has "localRootSpan" member, | ||
| // which we could probably use: that would allow omitting quite expensive | ||
| // parentSpanContextFromRef call. However, interpreting the pointer and | ||
| // the complex struct memory layout is more complicated and dangerous. | ||
| samplingState uintptr | ||
|
|
||
| // remote indicates that span context represents a remote parent | ||
| remote bool | ||
| } |
There was a problem hiding this comment.
This is, perhaps, the most controversial part. If anyone knows of a way to avoid such unsafe access, I'll be happy to make the necessary changes.
Please note that wrapping jaeger.SpanContext is not easily achievable, as this concrete type is often expected by users and is utilized in assertions.
There was a problem hiding this comment.
To me this seems fine, we could consider running our fork of open tracing while we transition off.
kolesnikovae
commented
Dec 8, 2023
Comment on lines
+75
to
+85
| // We create a span wrapper to ensure we remove the newly attached pprof | ||
| // labels when span finishes. The need of this wrapper is questioned: | ||
| // as we do not have the original context, we could leave the goroutine | ||
| // labels – normally, span is finished at the very end of the goroutine's | ||
| // lifetime, so no significant side effects should take place. | ||
| w := spanWrapper{ | ||
| parentPprofCtx: parentCtx, | ||
| currentPprofCtx: ctx, | ||
| } | ||
| w.Span = span.SetTag(profileIDTagKey, spanID) | ||
| return &w |
There was a problem hiding this comment.
It's worth noting that we could likely avoid wrapping jaeger.Span and preserve the original type. On the other hand, pprof does not clarify the behavior in case goroutine labels are not cleared (AFAIK).
kolesnikovae
requested review from
cyriltovena
and removed request for
cyriltovena
|
/cc @cyriltovena |
# Conflicts: # CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The PR introduces
spanprofiler. Its purpose is to facilitate "request-scoped" profiles with minimal effort and impact on application performance. The use of OpenTracing and OpenTelemetry SDK interfaces appears to be the simplest solution to this problem. For a detailed description, please refer to the README note.We've successfully integrated and tested this feature in our internal deployments over the past two months with no reported issues.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]