-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
fix: OTel spans for Sentry requests #3001
Conversation
@@ -113,7 +113,7 @@ public override void OnEnd(Activity data) | |||
// Make a dictionary of the attributes (aka "tags") for faster lookup when used throughout the processor. | |||
var attributes = data.TagObjects.ToDict(); | |||
|
|||
if (attributes.TryGetTypedValue("http.url", out string? url) && (_options?.IsSentryRequest(url) ?? false)) | |||
if (attributes.TryGetTypedValue("url.full", out string? url) && (_options?.IsSentryRequest(url) ?? false)) |
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.
this approach to detect if it's a Sentry request seems rather fragile
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.
Other ideas?
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.
AsyncLocal?
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.
Sorry, should have put this in the original review... might be good to change this test from a Fact to a Theory and test it with both the old and the new:
sentry-dotnet/test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs
Lines 381 to 382 in 41714ae
[Fact] | |
public void OnEnd_IsSentryRequest_DoesNotFinishTransaction() |
At the moment, it only tests the old attributes.
Fixes #2998
Looks like the key changed.