-
-
Notifications
You must be signed in to change notification settings - Fork 465
Assume http.client for span op if not a root span
#4257
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
Conversation
Performance metrics 🚀
|
lcian
left a comment
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.
LGTM!
| @SuppressWarnings("deprecation") | ||
| public @NotNull OtelSpanInfo extractSpanInfo( | ||
| final @NotNull SpanData otelSpan, final @Nullable IOtelSpanWrapper sentrySpan) { | ||
| if (!isInternalSpanKind(otelSpan)) { |
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.
Could you explain why we removed this check?
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.
The problem we had before (see #3904) was that when span kind was set to neither CLIENT nor SERVER we would generate http as op (without .client or .server suffix).
The default span kind is INTERNAL. So if a customer manually instruments using OTel, doesn't set span kind and has HTTP span attributes set, the SpanDescriptionExtractor wouldn't know whether it's an incoming or outgoing request.
Having this check in place would prevent the changes in this PR from taking effect.
lbloder
left a comment
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.
LGTM 👍
📜 Description
When extracting span description, assume it's
http.clientif the span is not a root span (invalid or remote parent).💡 Motivation and Context
Fixes #3904
💚 How did you test it?
📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps