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

feat: Extract normalized dist as metric [INGEST-335] #1158

Merged
merged 5 commits into from
Dec 16, 2021

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Dec 15, 2021

Apply normalization to event.dist and add as tag to every transaction metric.

There is no further normalization in sentry python code as far as I can see: The dist is accepted in event_manager unaltered:

https://github.com/getsentry/sentry/blob/44d7aa211d67b64884230d833f6e485a96357ad6/src/sentry/event_manager.py#L674

if erase {
*dist = None;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Added the erase flag here because I cannot set dist while checking its contents. I don't like it though - any suggestions welcome.

@jjbayer jjbayer marked this pull request as ready for review December 15, 2021 16:08
@jjbayer jjbayer requested review from a team, untitaker and ahmedetefy December 15, 2021 16:08
relay-general/src/store/normalize.rs Outdated Show resolved Hide resolved
Co-authored-by: Markus Unterwaditzer <markus-honeypot@unterwaditzer.net>

#[test]
fn test_normalize_dist_empty() {
let mut dist = Option::Some("".to_owned());
Copy link
Member

Choose a reason for hiding this comment

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

my prev code suggestion goes for all testcases here fwiw

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I did the lazy thing and pressed "Commit suggestion". Will change other instances as well.

@jjbayer jjbayer merged commit 5f6f801 into master Dec 16, 2021
@jjbayer jjbayer deleted the feat/extract-transaction-dist branch December 16, 2021 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants