-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add outcomes to TraceItem #174
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5a06a80
feat: add ItemOutcomes message to TraceItem
MeredithAnya cc67ac5
refactor: rename ItemOutcomes to ItemOutcome (singular)
MeredithAnya e27427a
move key_id
MeredithAnya 1c457dc
outcomes, categorycount
MeredithAnya 6b136ec
chore: Regenerate Rust bindings
getsantry[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This should be an enum taken from the list of categories in the Relay package.
@Dav1dde should we extract this from Relay?
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.
@phacops I was making the assumption we would validate the data category in the consumer itself
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's fine. I guess we also don't have a list of
DataCategoryin protobuf so what I'd want to do is not really possible yet. We'd have to make the list in protobuf first then replace it everywhere it's used instead of using the Python class to generate it or something.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.
@Dav1dde can we get a rust crate for the relay consts? so we can import the consts like we do in other places in snuba
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.
In Relay, for outcomes, the data category is actually typed as
u8which leaves us with about 220 more data categories we can invent.Currently the source of truth for data categories lives in Sentry, but also all of Sentry (and Relay for that matter) is built in a way that it needs to be able to deal with unknown datacategories (arbitrary numbers), so we could follow this approach also here and just use a number.
I think setting up code generation in Relay, to import into sentry-protos creates a weird dependency graph where sentry-protos depends on some Relay artifcat but Relay also depends on sentry-protos to produce data.
Is changing from
u8/uint32to anenumin protobuf a breaking change? If not we can start out as a number, then think about moving the source of truth for a data category into sentry-protos or some other schema repo.Uh oh!
There was an error while loading. Please reload this page.
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.
Maybe I misunderstood the question, @MeredithAnya would you like to use the rust artifact/list of datacateories just in the consumer to have some typing or also here in the protos?
If it's just about the consumer, the consumer needs to be able to deal with unknown data categories, as we don't want to have to update the consumer before introducing a new data category.
Uh oh!
There was an error while loading. Please reload this page.
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.
@Dav1dde I was talking about the consumers. Right now I want to enforce that the data categories we are creating accepted outcomes for are in the right subset, in getsentry outcomes consumer they are using it like
where in sentry's
constants.pytheDataCategoryisObviously I'd be doing this differently but right now snuba only has the ability to do this in the python consumers/ api logic with https://pypi.org/project/sentry-relay/