-
-
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(apis): Pass query source to snuba - phase 2 #73497
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #73497 +/- ##
=======================================
Coverage 78.19% 78.19%
=======================================
Files 6790 6790
Lines 302551 302602 +51
Branches 52057 52062 +5
=======================================
+ Hits 236585 236627 +42
- Misses 59581 59588 +7
- Partials 6385 6387 +2
|
711df0a
to
d3154bf
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.
Mostly looks good to me, though it would be ideal if the logic to detect if the query source can be collected in 1 place instead of having some endpoints override it with some custom logic.
# TODO: @athena - add query_source when all datasets support it | ||
# query_source=( | ||
# QuerySource.FRONTEND if is_frontend_request(request) else QuerySource.API | ||
# ), |
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.
Did you mean to leave this commented out?
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.
Yes, not all datasets currently expect query_source as a param and they're not typed to safely take care of that. So I'll uncomment it after my last PR.
if referrer in SENTRY_BACKEND_REFERRERS: | ||
query_source = QuerySource.SENTRY_BACKEND |
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.
Can this logic be moved into is_frontend_request
? Also, is there no other way to verify that the request came from the backend by inspecting the request like the authentication method?
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_frontend_request
is using authentication but really no matter how we implement it a customer's direct api call can look like a frontend request so this is just a good estimate.
I don't think it's a good idea to move this to is_frontend_request because that's really independent of Snuba. Let me create a method in Endpoint
or a util 🤔
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
d3154bf
to
fddbf48
Compare
1454c07
to
a2120cd
Compare
Continuing #73497 to pass query source to snuba for more debuggability. OrganizationEventsTraceBaseEndpoint OrganizationsEventsNewTrendsStatsEndpoint OrganizationsEventsEndpoint
Continuing #73176 to pass query source to snuba for more debuggability.
OrganizationSpansSamplesEndpoint
OrganizationsEventsMetaEndpoint
OrganizationsEventsStatsEndpoint