-
-
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(dynamic-sampling): add endpoint to retrieve project span counts #80587
feat(dynamic-sampling): add endpoint to retrieve project span counts #80587
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #80587 +/- ##
==========================================
+ Coverage 78.36% 78.39% +0.02%
==========================================
Files 7206 7216 +10
Lines 318724 319330 +606
Branches 43907 43976 +69
==========================================
+ Hits 249778 250340 +562
- Misses 62585 62616 +31
- Partials 6361 6374 +13 |
projects = cast( | ||
Sequence[Project], | ||
Project.objects.filter(organization=organization, status=ObjectStatus.ACTIVE).all(), | ||
) |
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 a potential issue. Queryset .all()
does nothing but copy the queryset, which is basically an iterator and not really a sequence. typing.cast
is essentially a no-op functionally, just there for typing. If you require an actual sequence I would use projects = list(Projects.objects.filter(...))
.
projects = cast( | ||
Sequence[Project], | ||
list( | ||
Project.objects.filter(organization=organization, status=ObjectStatus.ACTIVE).all() | ||
), | ||
) |
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.
Cast to Sequence
should no longer be required - a list is a sequence. Also, list()
will evaluate the queryset, I think you can omit the .all()
.
Optionally, I'd add a comment to clarify that we do not want team filters even with closed membership, so that in the future this is not unintentionally "fixed".
projects = cast( | |
Sequence[Project], | |
list( | |
Project.objects.filter(organization=organization, status=ObjectStatus.ACTIVE).all() | |
), | |
) | |
projects = list( | |
Project.objects.filter(organization=organization, status=ObjectStatus.ACTIVE) | |
) |
src/sentry/api/urls.py
Outdated
@@ -1417,6 +1420,11 @@ def create_group_urls(name_prefix: str) -> list[URLPattern | URLResolver]: | |||
OrganizationSamplingProjectRatesEndpoint.as_view(), | |||
name="sentry-api-0-organization-sampling-project-rates", | |||
), | |||
re_path( | |||
r"^(?P<organization_id_or_slug>[^\/]+)/sampling/project-span-counts/$", |
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 endpoint returns the span count per root project, which distinguishes it from regular stats endpoints. Should we choose a URL that makes this clear? This might make the URL too long, so I'm not sure we should actually do this.
Options could be:
project-root-counts
(we can equally add other counts there if we need to)span-root-counts
root-counts
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.
Yeah that's a good point. None of them really feel intuitive to me, but I suppose project-root-counts
is probably the best, as it even takes out the specificity of spans in case we ever adapt what type of data sampling acts on.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Contributes to https://github.com/getsentry/projects/issues/353