fix(tracing) Omit the top level status tag#644
Conversation
In the product side we don't really want people to search by this tag as it is far more expensive than the `transaction.status` property which is indexed separately. By not emitting this tag and only including it in the trace context we won't end up with poor performing tags for users to click on.
untitaker
left a comment
There was a problem hiding this comment.
@kamilogorek @HazAT can you make sure this happens for JS
|
We never did any of this in JS 🤔 Edit: Removed out of place "string" word xD (was coding beside writing this) |
|
What do you mean with precaution string? We used to send this: with this change we remove the event tag |
|
@untitaker OK, updated my comment. |
To workaround get_trace_context() being called multiple times I needed an additional non-tags place to store the status. I've had to shim the status back into tags as non-transaction spans expect to have status as a tag.
|
I had to add a status attribute to the Span object. Without that transactions collected inside celery would not have statuses set because |
|
@HazAT I am looking at the UI and I do see JS transactions having a status tag. Could you please figure out what logic to apply here? Do we want to get rid of tags altogether, etc |
|
we found out it wroks exactly the same in JS: https://github.com/getsentry/sentry-javascript/blob/712b6e7de4a5af6449e651915ce73984751105b7/packages/apm/src/span.ts#L308 However, we do not set the status explicitly most of the time, therefore there rarely is a top-level tag in JS |
In the product side we don't really want people to search by this tag as it is far more expensive than the
transaction.statusproperty which is indexed separately. By not emitting this tag and only including it in the trace context we won't end up with poor performing tags for users to click on.