-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(Activated Alerts): implement deploy activator #70712
Conversation
🚨 Warning: This pull request contains Frontend and Backend changes! It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently. Have questions? Please ask in the |
Bundle ReportChanges will increase total bundle size by 57 bytes ⬆️
|
8f58891
to
b3d024d
Compare
b3d024d
to
ea4a677
Compare
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.
just the question about query_extra vs activator - code seems good, please make sure to add some tests around the manager (now will be the easiest time to add them)
query_extra = f"release:{release.version} and environment:{environment.name}" | ||
# TODO: parse activator on the client to derive release version / environment name | ||
activator = f"release:{release.version} and environment:{environment.name}" |
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.
nit: rather than storing as text blobs, thoughts on building this string in the UI and storing release / env in json? (unclear how large of a refactor this ask is though).
Also, curious why we need to replicate this data? Should the activator only store activation reasons, and we remove activation info from query_extra? My understanding is that query_extra is meant to be meta data about the query itself stored in text. The activator is meant to be the release / deploy / environment information that triggered the alert on activated alerts.
Do we need to do anything to anticipate deploy version info here?
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.
Yea - query_extra is actually being tacked on to the query.
activator
is simply something we can provide to the client to help better understand what triggered the monitor.
For now i mirrored it with the query_extra, btu we could make this any string 🤷
Deploy version isn't valuable and we do not surface it anywhere else in the UI either :(
'deploy's are simply releases on certain environments, and that's how we surface it elsewhere so stuck wtih that
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 trigger isn't being generated from any UI, so we cannot build anything in the UI / have nothing to store in JSON
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.
I was more thinking that the format could be store as json rather than a string - incase we want to parse it later or share it via an API. it's just a nice to have that might make the future a little easier. Basically, just doing: json.dumps({ release: release.version, environment: environent.name })
then we can parse it with json.loads()
on fetch. Just thinking it might be nicer to interact with this as a dictionary than as a string incase we need the release / env name later, but isn't a blocker.
The thing i was thinking with deploys is that in the future we might want to have an activator be a specific deploy, but def don't need to get hung-up on it for the mvp either - so :punt:
Yup looking at tests now |
query_extra = f"release:{release.version} and environment:{environment.name}" | ||
# TODO: parse activator on the client to derive release version / environment name | ||
activator = f"release:{release.version} and environment:{environment.name}" |
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.
I was more thinking that the format could be store as json rather than a string - incase we want to parse it later or share it via an API. it's just a nice to have that might make the future a little easier. Basically, just doing: json.dumps({ release: release.version, environment: environent.name })
then we can parse it with json.loads()
on fetch. Just thinking it might be nicer to interact with this as a dictionary than as a string incase we need the release / env name later, but isn't a blocker.
The thing i was thinking with deploys is that in the future we might want to have an activator be a specific deploy, but def don't need to get hung-up on it for the mvp either - so :punt:
|
||
assert mock_subscribe_project_to_alert_rule.call_count == 1 | ||
|
||
@patch( |
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.
question about sentry testing philosophy... i've seen mocking sparingly and mostly checks for the db to have the conditions completed afterwards. is there a general recommendation?
(I personally like mocking out external logic like this because it improves test performance so much.. but the trade-off is testing implementation a bit more explicitly over the logic of the function)
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.
mmmm 🤔
I know we tend to like full e2e tests in our unit tests....
Personally i'm also in favor of testing the individual units and mocking the relations... IMO makes it easier to more clearly understand whats going on and whats expected?
that being said - i have the non patched tests below as well 😂
hmmmm yea- i considered the JSON as well it would definitely make it easier to parse for the client. but i'm a little sus of sticking JSON into the db... for now it's just a short charfield, so if we want to enable JSON we'd need to update the column with a bigger size 😬 |
4776eff
to
380c36c
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Deploy reference a specific release being deployed to a specific environment
To activate a monitor for a deploy means to start monitoring per release AND environment for a specific Project.
This PR subscribes the monitor subscription to the creation of a
ReleaseProjectEnvironment
which for all intents and purposes is synonymous with "deploy"