Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(protocol): Add transaction_info to events [INGEST-1427] #1330

Merged
merged 8 commits into from
Jul 4, 2022

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Jul 1, 2022

Implements transaction_info as documented in getsentry/develop#624 and adds the event.transaction_source metric to measure how often each source occurs in practice.

Requires #1329

@jan-auer jan-auer requested a review from a team July 1, 2022 11:48
@jan-auer jan-auer self-assigned this Jul 1, 2022
@jan-auer jan-auer changed the title feat(protocol): Add transaction_source to events [INGEST-1427] feat(protocol): Add transaction_info to events [INGEST-1427] Jul 1, 2022
@untitaker
Copy link
Member

please also add a statsd metric or whatever else we need to be able to view aggregates of this data

Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for adding a statsd metric right away.


/// The unmodified transaction name as obtained by the source.
///
/// This value will only be set if the transaction name was modified during event processing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, don't we have _meta for original data?

Copy link
Member Author

@jan-auer jan-auer Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the DACI it was decided not to use _meta to make the information more accessible to the product.


metric!(
counter(RelayCounters::EventTransactionSource) += 1,
source = &source.to_string()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to tag this by platform and/or SDK version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great suggestion, going to add this before merging.

@jan-auer jan-auer merged commit 135eb86 into master Jul 4, 2022
@jan-auer jan-auer deleted the feat/transaction-source branch July 4, 2022 09:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants