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(tracing): Make sure a valid source is set on a transaction event #637

Merged
merged 3 commits into from
May 15, 2023

Conversation

tonyo
Copy link
Contributor

@tonyo tonyo commented May 9, 2023

Transaction source should be one of the predefined values as described by the envelopes protocol: https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations

The safest way to do for Go seems to be checking the source in Span.toEvent(): we don't currently have setters for Span.Name and Span.Source, so adjusting the source earlier may be not enough.

Fixes #588
Related to #511

@tonyo tonyo requested a review from cleptric May 9, 2023 14:26
@tonyo tonyo self-assigned this May 9, 2023
@tonyo tonyo marked this pull request as draft May 9, 2023 14:30
@tonyo tonyo removed the request for review from cleptric May 9, 2023 14:30
@tonyo
Copy link
Contributor Author

tonyo commented May 9, 2023

Now I'm looking at this again, lemme think a bit more about making Span.Source private 🤔

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

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

Comparison is base (5d969d0) 79.60% compared to head (328ce62) 79.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #637      +/-   ##
==========================================
+ Coverage   79.60%   79.64%   +0.04%     
==========================================
  Files          38       38              
  Lines        3888     3896       +8     
==========================================
+ Hits         3095     3103       +8     
  Misses        683      683              
  Partials      110      110              
Impacted Files Coverage Δ
tracing.go 89.43% <100.00%> (+0.17%) ⬆️

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

@tonyo tonyo requested a review from cleptric May 9, 2023 14:43
@tonyo tonyo marked this pull request as ready for review May 9, 2023 14:44
@tonyo
Copy link
Contributor Author

tonyo commented May 9, 2023

Decided not to touch Span.Source for now: we should probably couple that with overall improvements around settings/getting transaction Source and Name.

@tonyo tonyo merged commit eec094e into master May 15, 2023
@tonyo tonyo deleted the tonyo/fix-fixing-transaction-source branch May 15, 2023 11:05
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.

Invalid transaction source: echo
2 participants