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: Introduce a new Span.Name field #605

Merged
merged 3 commits into from Mar 21, 2023
Merged

Conversation

cleptric
Copy link
Member

@cleptric cleptric commented Mar 21, 2023

This removes the transaction on the scope in favor of a new Span Name field.
Besides fixing an issue with the Otel context, we also gain a cleaner way of accessing the transaction name, for example in the TracesSampler:

Before

TracesSampler: func(ctx sentry.SamplingContext) float64 {
	hub := sentry.GetHubFromContext(ctx.Span.Context())
	if hub.Scope().Transaction() == "GET /health" {
		return 0
	}
	return 1
},

After

TracesSampler: func(ctx sentry.SamplingContext) float64 {
	if ctx.Span.Name == "GET /health" {
		return 0
	}
	return 1
},

closes #596

@cleptric cleptric self-assigned this Mar 21, 2023
@cleptric cleptric linked an issue Mar 21, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.12 🎉

Comparison is base (dc573d7) 78.86% compared to head (cb8009a) 78.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
+ Coverage   78.86%   78.99%   +0.12%     
==========================================
  Files          38       38              
  Lines        3870     3851      -19     
==========================================
- Hits         3052     3042      -10     
+ Misses        714      708       -6     
+ Partials      104      101       -3     
Impacted Files Coverage Δ
scope.go 92.14% <ø> (-0.43%) ⬇️
dynamic_sampling_context.go 93.42% <100.00%> (ø)
otel/span_processor.go 94.33% <100.00%> (+2.52%) ⬆️
tracing.go 88.79% <100.00%> (+0.62%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@cleptric cleptric marked this pull request as ready for review March 21, 2023 11:33
@cleptric cleptric merged commit 85b380d into master Mar 21, 2023
15 checks passed
@cleptric cleptric deleted the otel-transaction-name branch March 21, 2023 13:00
@pbennett
Copy link

pbennett commented Apr 2, 2023

So how do we override the transaction name in later calls?
I do this currently:

sentry.GetHubFromContext(ctx).Scope().SetTransaction("xxxx")

This is no longer possible.

@cleptric
Copy link
Member Author

cleptric commented Apr 2, 2023

transaction := sentry.TransactionFromContext(ctx)
if transaction != nil {
	transaction.Name = "xxxx"
}

@jerbob92
Copy link

jerbob92 commented Apr 4, 2023

How would this work for other/error events? We automatically create and inject a Hub per request and then do:

		hub.ConfigureScope(func(scope *sentry.Scope) {
			scope.SetTransaction(request_info.Id)
		})

This would then automatically add the transaction to every event coming from this scope using the ApplyToEvent method, but the lines that added it are gone:

	if scope.transaction != "" {
		event.Transaction = scope.transaction
	}

And it doesn't look an alternative has been added?

Edit: it could also be that I'm using the Transaction field completely wrong. Should I move the Request ID to something else? Like a tag or context?

@cleptric
Copy link
Member Author

cleptric commented Apr 4, 2023

@jerbob92 So, transaction on the event has a long history, even before Sentry had a performance product.
There might be some legit use cases for some users that only use our error product, but in your case, a tag would give you the same UX in the product. Tags are indexed, hence searchable and appear in the UI in the same place as transaction did.

@pbennett
Copy link

pbennett commented Apr 4, 2023

Perhaps updating the documentation and SDKs are in order because frankly, transactions / spans / tags / things that show as 'database' events are all super obscure and vary from sdk to sdk.

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.

OTel Context Missing
4 participants