Skip to content

fix: Introduce a new Span.Name field#605

Merged
cleptric merged 3 commits intomasterfrom
otel-transaction-name
Mar 21, 2023
Merged

fix: Introduce a new Span.Name field#605
cleptric merged 3 commits intomasterfrom
otel-transaction-name

Conversation

@cleptric
Copy link
Copy Markdown
Member

@cleptric cleptric commented Mar 21, 2023 •
edited
Loading

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
OTel Context Missing #596
Closed
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2023 •
edited
Loading

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 force-pushed the otel-transaction-name branch from f3aaa57 to cb8009a Compare March 21, 2023 11:43
@cleptric cleptric merged commit 85b380d into master Mar 21, 2023
@cleptric cleptric deleted the otel-transaction-name branch March 21, 2023 13:00
@tonyo tonyo mentioned this pull request Mar 28, 2023
Open
7 tasks
@pbennett
Copy link
Copy Markdown

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
Copy Markdown
Member Author

cleptric commented Apr 2, 2023 •
edited
Loading 

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

@kei2100 kei2100 mentioned this pull request Apr 4, 2023
Closed
@jerbob92
Copy link
Copy Markdown

jerbob92 commented Apr 4, 2023 •
edited
Loading

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
Copy Markdown
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
Copy Markdown

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

Reviewers

1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cleptric cleptric

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OTel Context Missing

4 participants

@cleptric @pbennett @jerbob92 @AbhiPrasad