-
-
Notifications
You must be signed in to change notification settings - Fork 452
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
Assume http.client
for span op
if not a root span
#4257
Conversation
Performance metrics 🚀
|
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!
@@ -18,23 +18,16 @@ public final class SpanDescriptionExtractor { | |||
@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.
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.client
if the span is not a root span (invalid or remote parent).💡 Motivation and Context
Fixes #3904
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps