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

ref(general): Do not default transactions to level error #585

Merged
merged 2 commits into from
May 20, 2020

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented May 20, 2020

In transactions, the error event level does not make sense. Instead, assign info to transactions.

@jan-auer jan-auer requested review from HazAT and a team May 20, 2020 08:42
@jan-auer jan-auer self-assigned this May 20, 2020
Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

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

we'd have to test locally if this breaks later stages of the pipeline

thanks for taking this, it's been in my personal backlog for a while

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

🙏 Thx

@jan-auer
Copy link
Member Author

@untitaker you were right, Sentry unconditionally writes a None tag, which leads to an error in Snuba/clickhouse.

@HazAT I'm defaulting to "info" now.

@jan-auer jan-auer requested review from HazAT and untitaker May 20, 2020 09:14
@untitaker
Copy link
Member

untitaker commented May 20, 2020

Sentry unconditionally writes a None tag, which leads to an error in Snuba/clickhouse.

so if it's about the tag can't we get rid of the tag, or never set it if it's none?

@jan-auer
Copy link
Member Author

jan-auer commented May 20, 2020

so if it's about the tag can't we get rid of the tag, or never set it if it's none?

Yes, but I'd like to do this as a follow-up in Sentry. @untitaker @HazAT would you prefer this to be fixed in Sentry first and then we go back to the prior patch? I personally like that it has a level always, since there's no straight forward way to query in snuba for a missing value.

I forgot to update tests, so depending on your decision I'll either roll back or fix tests.

@untitaker
Copy link
Member

there's no straight forward way to query in snuba for a missing value

That I agree with but I don't think we need to solve it this way. Let the discover team figure out how to query for that.

event.level.get_or_insert_with(|| match event_type {
EventType::Transaction => Level::Info,
_ => Level::Error,
});
Copy link
Member

Choose a reason for hiding this comment

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

This is great 👍

@dashed
Copy link
Member

dashed commented May 20, 2020

@jan-auer @untitaker It's a non-issue of whether or not transactions has a level field/tag. We don't rely on this anywhere on the product.

The data team might care about this change as it could impact their work. I'm unsure if they rely on this field/tag to make certain assumptions about transactions events.

@jan-auer jan-auer merged commit 8a22667 into master May 20, 2020
@jan-auer jan-auer deleted the fix/transaction-default-level branch May 20, 2020 10:43
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.

4 participants