-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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(alerts): Create eap snuba subscription using rpc timeseries request #80665
feat(alerts): Create eap snuba subscription using rpc timeseries request #80665
Conversation
…lerts-create-eap-rpc-subscription
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #80665 +/- ##
==========================================
+ Coverage 78.35% 78.38% +0.02%
==========================================
Files 7212 7218 +6
Lines 319167 319627 +460
Branches 43946 43999 +53
==========================================
+ Hits 250082 250538 +456
+ Misses 62714 62711 -3
- Partials 6371 6378 +7 |
2e0d2bb
to
deb0510
Compare
deb0510
to
b3b7a3f
Compare
endpoint_name: str, class_version: str, req: SnubaRPCRequest | ||
endpoint_name: str, | ||
class_version: str, | ||
req: SnubaRPCRequest | CreateSubscriptionRequest, |
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.
Currently, the CreateSubscriptionRequest
protobuf doesn't have a meta
attribute with a referrer
. I've added CreateSubscriptionRequest
as a valid type here to handle these scenarios. I'm open to updating the protobuf to include meta
instead if we feel that would be better
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 think this is fine for now, I'll clean this up in a future pass 👍
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.
lgtm, but i'm not that familiar with the alerts code
endpoint_name: str, class_version: str, req: SnubaRPCRequest | ||
endpoint_name: str, | ||
class_version: str, | ||
req: SnubaRPCRequest | CreateSubscriptionRequest, |
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 think this is fine for now, I'll clean this up in a future pass 👍
Updates Alert Rule creation for EAP alerts to use the Create Subscription RPC endpoint in snuba (getsentry/snuba#6499)
Constructs and passes a
TimeSeriesRequest
to be used in the subscription creation.