Skip to content
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(ddm): Add new metrics/query endpoint base code #64785

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Feb 7, 2024

This PR adds a new /metrics/query endpoint and copies over all the querying/data files in querying/data_v2 to kickstart the development of the updated metrics API.

Closes: #64769

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 7, 2024
@@ -0,0 +1,61 @@
from collections.abc import Sequence
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied the entire code in data to make the new changes completely independent of the old ones. Should enable us to keep the old endpoint up and running without problems.

referrer=Referrer.API_DDM_METRICS_QUERY.value,
)
except InvalidMetricsQueryError as e:
return Response(status=400, data={"detail": str(e)})

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
except InvalidMetricsQueryError as e:
return Response(status=400, data={"detail": str(e)})
except LatestReleaseNotFoundError as e:
return Response(status=404, data={"detail": str(e)})

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
except LatestReleaseNotFoundError as e:
return Response(status=404, data={"detail": str(e)})
except MetricsQueryExecutionError as e:
return Response(status=500, data={"detail": str(e)})

Check warning

Code scanning / CodeQL

Information exposure through an exception Medium

Stack trace information
flows to this location and may be exposed to an external user.
@@ -201,6 +201,15 @@ class OrgAuthTokenPermission(OrganizationPermission):
}


class OrganizationMetricsPermission(OrganizationPermission):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed different permissions since by default we don't have org:read on POST.

# TODO: figure out how to make these methods work with HTTP body.
projects=self.get_projects(request, organization),
environments=self.get_environments(request, organization),
referrer=Referrer.API_DDM_METRICS_QUERY.value,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a different referrer to separately track in snuba the usage of the new endpoint.

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (aa2e6f2) 81.36% compared to head (ebe4e19) 81.37%.
Report is 70 commits behind head on master.

❗ Current head ebe4e19 differs from pull request most recent head c414a95. Consider uploading reports for the commit c414a95 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #64785      +/-   ##
==========================================
+ Coverage   81.36%   81.37%   +0.01%     
==========================================
  Files        5236     5243       +7     
  Lines      231130   231453     +323     
  Branches    45348    45400      +52     
==========================================
+ Hits       188058   188347     +289     
- Misses      37220    37252      +32     
- Partials     5852     5854       +2     
Files Coverage Δ
src/sentry/api/bases/organization.py 91.15% <100.00%> (+0.06%) ⬆️
src/sentry/api/urls.py 100.00% <ø> (ø)
...sentry/sentry_metrics/querying/data_v2/__init__.py 100.00% <100.00%> (ø)
src/sentry/snuba/referrer.py 100.00% <100.00%> (ø)
src/sentry/sentry_metrics/querying/data_v2/api.py 75.75% <75.75%> (ø)
src/sentry/api/endpoints/organization_metrics.py 81.86% <55.76%> (-9.69%) ⬇️

... and 58 files with indirect coverage changes

Copy link
Member

@obostjancic obostjancic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline. LGTM

@iambriccardo iambriccardo enabled auto-merge (squash) February 8, 2024 09:18
@iambriccardo iambriccardo merged commit f4541c8 into master Feb 8, 2024
48 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/multi-series-support branch February 8, 2024 09:45
Copy link

sentry-io bot commented Feb 16, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ InvalidParams: start must be before end /api/0/organizations/{organization_slug}/metric... View Issue
  • ‼️ InvalidParams: Invalid statsPeriod: '90d-trial' /api/0/organizations/{organization_slug}/metric... View Issue

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create new metrics/query endpoint
2 participants