Skip to content
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: Improve isSentryRequestUrl #632

Merged
merged 1 commit into from May 4, 2023
Merged

Conversation

cleptric
Copy link
Member

@cleptric cleptric commented May 4, 2023

If we can't fetch a hub from the ctx, we now fallback to the global hub.
I also added the project id to not solely rely on the hostname.

@cleptric cleptric requested a review from tonyo May 4, 2023 15:22
@cleptric cleptric self-assigned this May 4, 2023
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: +0.03 🎉

Comparison is base (eb7fca9) 79.90% compared to head (eafed26) 79.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
+ Coverage   79.90%   79.94%   +0.03%     
==========================================
  Files          38       38              
  Lines        3887     3889       +2     
==========================================
+ Hits         3106     3109       +3     
  Misses        673      673              
+ Partials      108      107       -1     
Impacted Files Coverage Δ
otel/internal/utils/sentryrequest.go 63.63% <20.00%> (-6.37%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -27,7 +27,10 @@ func IsSentryRequestSpan(ctx context.Context, s trace.ReadOnlySpan) bool {
func isSentryRequestUrl(ctx context.Context, url string) bool {
hub := sentry.GetHubFromContext(ctx)
if hub == nil {
return false
hub = sentry.CurrentHub()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: for better experience and to fix more cases like this, we might want to set the Hub on context a lot earlier, maybe even in StartSpan.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might not know which hub to set on the ctx in StartSpan.
I think it might be worth digging a little bit into how Otel handles go routines.
For now, we might still consider this as a quick win.

@cleptric cleptric merged commit f7478f0 into master May 4, 2023
13 of 14 checks passed
@cleptric cleptric deleted the fix-sentry-otel-requests branch May 4, 2023 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants