-
Notifications
You must be signed in to change notification settings - Fork 211
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
feat: http client integration #876
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #876 +/- ##
==========================================
+ Coverage 82.85% 83.06% +0.21%
==========================================
Files 55 56 +1
Lines 4619 4677 +58
==========================================
+ Hits 3827 3885 +58
Misses 636 636
Partials 156 156 ☔ View full report in Codecov by Sentry. |
@aldy505 We should document this (in _examples, and perhaps sentry-docs?), and also update changelog accordingly. |
httpclient/sentryhttpclient.go
Outdated
|
||
span := sentry.StartSpan(ctx, "http.client", sentry.WithTransactionName(fmt.Sprintf("%s %s", request.Method, cleanRequestURL))) | ||
|
||
for k, v := range s.tags { |
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 we just set the span.Tags instead of ranging through these, and setting one by one?
This is the 'right' way to do it, but it involves checking for nil map and dealing with mutex.
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.
Lol good catch. I was copying directly from this PR aldy505/sentry-integration#1
httpclient/sentryhttpclient.go
Outdated
span := sentry.StartSpan(ctx, "http.client", sentry.WithTransactionName(fmt.Sprintf("%s %s", request.Method, cleanRequestURL))) | ||
span.Tags = s.tags | ||
defer span.Finish() |
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.
We do not want to always start transactions here. If one already exists, start a child span.
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.
Just to be clear: http.client spans should only created if there is any parent span?
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.
Yes, Sentry does not work well with these kind of single span transactions. We can revisit this once span streaming is fully supported in the product, but this will take a few months.
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.
Ah got it.
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.
https://sentry.io/insights/http/ should also work with these spans fwiw, docs are here.
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.
*which you already mentioned in the PR description, nvm
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.
Another question (unrelated): Does that also affects all the span op within the insights module? Meaning not only http.client
, but db.sql
and messaging.*
also counts?
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.
I guess technically we extract all the required fields from a transaction and store it into the span data set, though this is not how these modules where initially designed to work.
// Only create the `http.client` span only if there is a parent span. | ||
parentSpan := sentry.GetSpanFromContext(request.Context()) | ||
if parentSpan == nil { | ||
return s.originalRoundTripper.RoundTrip(request) | ||
} |
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.
A quick note why I prefer this instead of creating a sentry.WithOnlyIfParentExists()
(a SpanOption
type):
I used the JS SDK a lot at work lately, and I've been wondering the entire week, on their startSpan
function, they have this as an option: https://github.com/getsentry/sentry-javascript/blob/216aaeba1ee27cce8a4876e1f9212ba374eb30b3/packages/types/src/startSpanOptions.ts#L14-L15
Should the Go SDK move forward with that, as a SpanOption
or not?
We could to the SpanOption
thing, but it feels really weird since the way Go SDK handles scopes/hubs with span is to add the span pointer into the scope. See
Lines 196 to 199 in a6acd05
if clientOptions.EnableTracing { | |
hub := hubFromContext(ctx) | |
hub.Scope().SetSpan(&span) | |
} |
Although we could just not call the scope.SetSpan()
function, I don't know about the behavior if this span that we're creating will ever have a child span being created manually by the user.
An effort to implement this: https://develop.sentry.dev/sdk/telemetry/traces/modules/requests/
Since we already have tracing without performance, this should be good to go.