-
-
Notifications
You must be signed in to change notification settings - Fork 494
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
Add transaction source annotation #1902
Conversation
545e1a5
to
0c051de
Compare
Codecov ReportBase: 98.42% // Head: 98.43% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## neel/baggage-dsc-continuation #1902 +/- ##
=================================================================
+ Coverage 98.42% 98.43% +0.01%
=================================================================
Files 150 150
Lines 9183 9195 +12
=================================================================
+ Hits 9038 9051 +13
+ Misses 145 144 -1
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 at Codecov. |
…nto neel/tx-source
@@ -18,7 +18,7 @@ class Event | |||
event_id level timestamp | |||
release environment server_name modules | |||
message user tags contexts extra | |||
fingerprint breadcrumbs transaction |
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.
It looks like a TransactionEvent
-only attribute? If that's the case, can we put it there?
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 had it there first, but then moved it here because it's always coupled to the transaction
field.
@@ -13,7 +13,7 @@ | |||
|
|||
let(:client) { Sentry::Client.new(configuration) } | |||
let(:hub) do | |||
Sentry::Hub.new(client, subject) | |||
Sentry::Hub.new(client, Sentry::Scope.new) |
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.
🤦♂️ my bad
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.
👍
See https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations for spec.
This is necessary going forward because we need to separate high cardinality / low quality (for us, only raw urls from rack) to enable ingestion to do metrics aggregation without blowing up.
Note: This is independent in principle, but I'm basing it off the baggage changes since all of this needs to go out together anyway and some of the
Transaction
constructors changed and would cause merge conflicts otherwise.closes #1866