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(generic-metrics): Rename UseCaseKey to MetricPathKey #46575

Closed
wants to merge 3 commits into from

Conversation

ayirr7
Copy link
Member

@ayirr7 ayirr7 commented Mar 30, 2023

As we expand the generic metrics platform to take on new use cases alongside the existing Performance use case, we will be introducing a new notion of use case. Currently, this UseCaseKey is used for configuring and instantiating the metrics (release health) and generic metrics (performance) consumers/indexers. With the introduction of new use cases that will fall under generic metrics, we will need to rename UseCaseKey to something that will be closer to what it does -- determine which metrics path (metrics or generic metrics) traffic will be flowing into. That's what this PR does.

[Future, but related work:]
We would eventually like to rename the Performance entry to GenericMetrics. This will happen once we have a new use case onboarded. The approach for this will need to be decided on, since this enum is currently used to start up the generic metrics consumer, and configure and initialize it. It is also used throughout the message processing pipeline and the indexer.

The approach that I'm planning to use:

  • Add GenericMetrics to the MetricPathKey enum. Update all references to Performance in the code to now be GenericMetrics. In this same PR, add an override in the indexer CLI that will take the CLI input (Performance, currently) and convert it to GenericMetrics inside the entrypoint function.
  • Change the Ops command to now pass in GenericMetrics
  • Remove Performance from the indexer CLI

I'm planning on putting the above in separate PRs from this one, once we have a new use case onboarded and are ready to make this switch.

@@ -24,7 +24,7 @@ def get(self, request: Request, organization: Organization) -> Response:
organization_id=organization.id,
start=params["start"],
end=params["end"],
use_case_id=UseCaseKey.PERFORMANCE,
use_case_id=MetricPathKey.PERFORMANCE,
Copy link
Member

Choose a reason for hiding this comment

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

It's a little confusing that we still call these use_case_ids everywhere... is the plan to eventually change that too?

@lynnagara
Copy link
Member

@ayirr7
Copy link
Member Author

ayirr7 commented Apr 10, 2023

Are you also planning on migrating https://github.com/getsentry/sentry-kafka-schemas/blob/cfb239064878bb0a1a33f863da15ee88bb98767d/schemas/snuba-generic-metrics.v1.schema.json#L11 where it is still called use_case_id

No, this wouldn't change. The message payloads will contain the true use_case_id before being sent to Snuba so this name (use_case_id) is accurate.

@ayirr7
Copy link
Member Author

ayirr7 commented Apr 10, 2023

Update: We're going to hold off on making this change until UseCaseKey is switched to UseCaseID in all relevant places. See #46957 for context.)

@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2023

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 label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #46575 (6006600) into master (f1754cf) will increase coverage by 0.03%.
The diff coverage is 96.87%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #46575      +/-   ##
==========================================
+ Coverage   81.12%   81.16%   +0.03%     
==========================================
  Files        4853     4863      +10     
  Lines      204319   203958     -361     
  Branches    11130    11159      +29     
==========================================
- Hits       165755   165540     -215     
+ Misses      38318    38172     -146     
  Partials      246      246              
Impacted Files Coverage Δ
...sentry_metrics/consumers/indexer/slicing_router.py 84.74% <50.00%> (-3.39%) ⬇️
src/sentry/snuba/entity_subscription.py 89.34% <80.00%> (ø)
src/sentry/snuba/metrics/fields/base.py 90.87% <94.44%> (ø)
...ry/api/endpoints/organization_measurements_meta.py 90.47% <100.00%> (ø)
src/sentry/api/endpoints/organization_metrics.py 97.82% <100.00%> (+0.23%) ⬆️
src/sentry/ingest/billing_metrics_consumer.py 83.13% <100.00%> (ø)
src/sentry/release_health/metrics.py 95.62% <100.00%> (ø)
src/sentry/release_health/metrics_sessions_v2.py 96.00% <100.00%> (ø)
...c/sentry/release_health/release_monitor/metrics.py 93.93% <100.00%> (ø)
src/sentry/search/events/builder/metrics.py 90.61% <100.00%> (+0.34%) ⬆️
... and 17 more

... and 201 files with indirect coverage changes

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

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 label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Oct 19, 2023
@getsantry getsantry bot closed this Oct 27, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2023
@asottile-sentry asottile-sentry deleted the rename-usecasekey branch December 27, 2023 16:07
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 Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants