-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(eventsv2) Remove transactionSlug and groupSlug #14257
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
Conversation
The upcoming work to adopt discover style queries means that we can no longer have special slugs. We can also use `argMax()` in snuba to get to the latest event when viewing an aggregated view. This removes the special slug value handling, and relies entirely on eventSlug. Discover2 will require that all modal states need to be reachable from just the eventSlug and query state. This change will break pagination in the short term but I'll be working on new endpoints that will allow that behavior to be restored in the short term. Refs SEN-875
| // * project-slug:a_bare_string:oldest | ||
| // * project-slug:/some/otherpath:deadbeef | ||
| if (value && !/^(?:[^:]+):(?:[^:]+):(?:[^:]+|latest|oldest)$/.test(value)) { | ||
| if (value && !/^(?:[^:]+):(?:[a-f0-9]+)(?:\:latest|oldest)?$/.test(value)) { |
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.
Is the project slug part of that right? Can we have : in project slug? Is every other character there really valid? If you wanted to, could also enforce that event ID has a length of 32
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.
Project slugs can't have : in them. The slug generation done in project.save() removes the : from the slug.
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.
@markstory to double check, are the colons are removed on the django side?
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.
There is still one : left between the projectid and eventid. We need to retain that one as eventids are only unique within a project.
| 'fields': [ | ||
| ['argMax', ['id', 'timestamp'], 'latest_event'], | ||
| ], | ||
| }, |
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.
@markstory Heads up, the docs above the SPECIAL_FIELDS variable may need to be updated.
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 code is not long for this world. I'll get this updated in my next set of changes.
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.
💀
dashed
left a comment
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.
Looks good to me. 👍
I cherry picked this PR to #14225 and it works fine.
The upcoming work to adopt discover style queries means that we can no longer have special slugs. We can also use
argMax()in snuba to get to the latest event when viewing an aggregated view.This removes the special slug value handling, and relies entirely on eventSlug. Discover2 will require that all modal states need to be reachable from just the eventSlug and query state.
This change will break pagination in the short term but I'll be working on new endpoints that will allow that behavior to be restored in the short term.
Refs SEN-875