ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics #51776

john-z-yang · 2023-06-28T04:14:58Z

Refactoring usages of `UseCaseKey` into `UseCaseID`

After recent changes to the metrics platform, we are no longer using UseCaseKey to represent the use case of a metrics and are transitioning to UseCaseID.

This pr refactors instances of UseCaseKey so that in the future it can be deleted/deprecated (see pr 46575)

codecov · 2023-06-28T05:04:30Z

Codecov Report

Merging #51776 (bf993a1) into master (ffce497) will decrease coverage by 1.73%.
The diff coverage is 89.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #51776      +/-   ##
==========================================
- Coverage   79.32%   77.59%   -1.73%     
==========================================
  Files        4909     4909              
  Lines      205933   205995      +62     
  Branches    35187    35193       +6     
==========================================
- Hits       163364   159851    -3513     
- Misses      37574    41092    +3518     
- Partials     4995     5052      +57

Impacted Files	Coverage Δ
src/sentry/search/events/builder/metrics.py	`82.79% <ø> (-0.04%)`	⬇️
...atic/app/utils/analytics/replayAnalyticsEvents.tsx	`100.00% <ø> (ø)`
...atic/app/utils/replays/hooks/useReplayPageview.tsx	`0.00% <ø> (ø)`
static/app/views/onboarding/setupDocs.tsx	`67.64% <ø> (ø)`
static/app/views/projectInstall/platform.tsx	`100.00% <ø> (ø)`
static/app/views/replays/detail/trace/trace.tsx	`0.00% <ø> (ø)`
static/app/views/replays/details.tsx	`0.00% <ø> (ø)`
src/sentry/sentry_metrics/use_case_id_registry.py	`92.85% <66.66%> (ø)`
src/sentry/snuba/metrics/datasource.py	`90.62% <66.66%> (-5.36%)`	⬇️
src/sentry/snuba/entity_subscription.py	`87.32% <75.00%> (ø)`
... and 13 more

... and 325 files with indirect coverage changes

This reverts commit 1c40763.

iambriccardo

Great work! I have left a few comments that I think should be addressed iff we know that UseCaseID.SPANS will use the generic_metrics dataset. If not then just remove any check for SPANS in the code (limiting it to only TRANSACTIONS and SESSIONS), since we might have to support spans for the metrics layer in the future.

src/sentry/snuba/metrics/fields/base.py

src/sentry/snuba/metrics/query.py

src/sentry/snuba/metrics/query_builder.py

src/sentry/snuba/metrics/query.py

iambriccardo

LGTM, will wait for @RaduW's double check here.

RaduW

Really nice PR, thank you for doing it.

Let's please clarify the endpoint issue (below) .

One big concern I have is regarding changing the input/output of endpoints from "performance", "release-health" to "transactions" and "sessions".

I think we can't do that in one go.
If we want to do it we would have to:

first support both as input (in the endpoints)
change UI to send UseCaseIDs and accept both UseCaseIDs and UseCaseKeys
change endpoints to only accept UseCaseIDs and send UseCaseIDs
change UI to accept only UseCaseIDs

One small concern: I think we should not accept both UseCaseID and UseCaseKey in the sentry_metrics.utils.

I think you already handled all cases already so there is no need to keep the mess around.

RaduW · 2023-07-05T15:26:28Z

src/sentry/api/endpoints/organization_metrics.py

 try:
- return UseCaseKey(request.GET.get("useCase", "release-health"))
+ return UseCaseID(request.GET.get("useCase", "sessions"))
 except ValueError:
 raise ParseError(
- detail=f"Invalid useCase parameter. Please use one of: {[uc.value for uc in UseCaseKey]}"
+ detail=f"Invalid useCase parameter. Please use one of: {[uc.value for uc in UseCaseID]}"
 )


I don't think we want to do this in one shot (i.e. the UI will still use the strings "performance" and "release-health"
at least in the beginning).
We should convert the query params here from UseCaseKey to UseCaseID in here,
something like:

# at the top of the file from sentry.sentry_metrics.indexer.base import to_use_case_id ... try: use_case_key = UseCaseKey(request.GET.get("useCase", "release-health")) return to_use_case_id(use_case_key) except ValueError: raise ParseError( detail=f"Invalid useCase parameter. Please use one of: {[uc.value for uc in UseCaseKey]}" )```

Do you mean our API should accept the following for now

strings it accept enum it maps to

"performance" UseCaseID.TRANSACTIONS

"transactions" UseCaseID.TRANSACTIONS

"sessions" UseCaseID.SESSIONS

"release-health" UseCaseID.SESSIONS

Once this pr is deployed, and the front end changes made, we will change the API again so that it will accept:

strings it accept enum it maps to

"transactions" UseCaseID.TRANSACTIONS

"sessions" UseCaseID.SESSIONS

How does that sound?

Hey John,

If it was up to me I would just keep the external interface fixed ( i.e. emit and accept the strings "performance" and "release-health" .

What you propose is much cleaner, and also harder.

If we change our endpoint to also accept "transactions" and "sessions" there is no harm done, we are still backward compatible.

At that point in time we can also change the UI to both accept and send "transactions" and "sessions".

Changing the backend to emit (in the response) "transactions" and "sessions" or stop accepting "release-health" and "performance" in its parameters breaks backward compatibility and I'm not quite sure what is our policy on that.

We need to take in account that we potentially have external users that are using the API (all our endpoints are considered public API) and if we do that we'll break them ( for no good reason IMO).

Before breaking compatibility we should check if that's acceptable (not sure who we should ask).

As a compromise we could make the Backend accept "transactions" and "sessions" ( in addition to "release-health" and "performance" ) and maybe make the UI send "transactions" and "sessions" and also accept "transactions" and "sessions" in addition to "release-health" and "performance".

I see, thanks!

src/sentry/sentry_metrics/use_case_id_registry.py

RaduW · 2023-07-05T15:49:09Z

src/sentry/sentry_metrics/utils.py

 def reverse_resolve_tag_value(
- use_case_id: UseCaseKey, org_id: int, index: Union[int, str, None], weak: bool = False
+ use_case_id: Union[UseCaseID, UseCaseKey],
+ org_id: int,


I would not do it here ( but at the call site... as high as possible).

I would leave reverse_resolve_tag_value, reverse_resolve, reverse_resolve_weak, resolve... to take only UseCaseID.

If we allow them to take either UseCaseKey or UseCaseID we just moved the problem from the indexer here ( an improvment but not by much).

I agree with @RaduW on this. I would prefer to have all the downstream methods accept UseCaseId and perform coercions at top level if we don't want to also refactor there.

RaduW · 2023-07-05T16:13:50Z

src/sentry/snuba/metrics/query.py

@@ -341,7 +340,7 @@ def validate_end(self) -> None:
 def validate_granularity(self) -> None:
 # Logic specific to how we handle time series in discover in terms of granularity and interval
 if (
- self.use_case_key == UseCaseKey.PERFORMANCE
+ self.use_case_id == UseCaseID.TRANSACTIONS


@iambriccardo I'm not sure what are you suggesting ( at this level we shouldn't see a UseCaseKey anymore, everything should be UseCaseID, if we don't support spans here we should make it explicit).

src/sentry/snuba/metrics/query.py

RaduW · 2023-07-05T16:28:54Z

tests/sentry/api/endpoints/test_organization_metric_data.py

@@ -70,7 +70,7 @@ def test_incorrect_use_case_id_value(self):
 assert response.status_code == 400
 assert (
 response.json()["detail"]
- == "Invalid useCase parameter. Please use one of: ['release-health', 'performance']"
+ == f"Invalid useCase parameter. Please use one of: {[uc.value for uc in UseCaseID]}"


(as commented in the endpoint)

I don't think we should change the inputs of endpoints at this time.

john-z-yang · 2023-07-06T05:09:54Z

@RaduW and @iambriccardo do you guys mind giving me a hand with this single test that is failing?

tests/sentry/api/endpoints/test_organization_metric_tags.py::OrganizationMetricsTagsIntegrationTest::test_mri_metric_tags__in_region_silo
 - AssertionError: (500, b'{"detail":"Internal Error","errorId":null}')

I think I might have missed something really obvious but can't figure out why :(

RaduW · 2023-07-06T11:06:37Z

@john-z-yang
_fetch_tags_or_values_for_mri had a parameter changed from use_case_key to use_case_id but not all calls were refactored ( see commit above).

src/sentry/api/endpoints/organization_metrics.py

src/sentry/testutils/cases.py

iambriccardo

Left some minor nits

iambriccardo · 2023-07-10T07:31:48Z

src/sentry/snuba/metrics/fields/base.py

@@ -196,11 +196,11 @@ def _get_entity_of_metric_mri(
 raise InvalidParams

 entity_keys_set: frozenset[EntityKey]
- if use_case_id == UseCaseKey.PERFORMANCE:
+ if use_case_id is UseCaseID.TRANSACTIONS:


Why somewhere we are using == and here is? I would keep ==, what do you think?

I guess different people changed it in different ways.
I generally use == with the exception of when I check against None where I always try to use is.

is UseCaseId.Transaction is the correct usage according to PEP 8:

Comparisons to singletons like None should always be done with is or is not, never the equality operators.

https://peps.python.org/pep-0008/#programming-recommendations

iambriccardo · 2023-07-10T07:35:04Z

src/sentry/sentry_metrics/utils.py

 def reverse_resolve_tag_value(
- use_case_id: UseCaseKey, org_id: int, index: Union[int, str, None], weak: bool = False
+ use_case_id: Union[UseCaseID, UseCaseKey],
+ org_id: int,


I agree with @RaduW on this. I would prefer to have all the downstream methods accept UseCaseId and perform coercions at top level if we don't want to also refactor there.

obostjancic

LGTM 🚢

implement use case id/key compatibility in metrics

38da146

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 28, 2023

vercel bot deployed to Preview June 28, 2023 04:34 View deployment

implement use case id/key compatibility in org_metrics

3e2179c

john-z-yang force-pushed the john/remove-uck branch from 95c2068 to 3e2179c Compare June 28, 2023 04:37

vercel bot deployed to Preview June 28, 2023 04:40 View deployment

john-z-yang changed the title ~~implement use case id/key compatibility in metrics~~ ref(generic-metrics): Migrate UseCaseKey to UseCaseID Jun 28, 2023

fix tests

1c40763

vercel bot deployed to Preview June 28, 2023 17:57 View deployment

Revert "fix tests"

74cb79e

This reverts commit 1c40763.

vercel bot deployed to Preview June 28, 2023 18:25 View deployment

fix tests

37aaa27

vercel bot deployed to Preview June 29, 2023 16:41 View deployment

update cases.py

2616cca

vercel bot deployed to Preview June 29, 2023 17:25 View deployment

update test

caae53f

vercel bot deployed to Preview June 29, 2023 17:31 View deployment

fix get_series callsites

86157c7

vercel bot deployed to Preview June 29, 2023 17:37 View deployment

fix mqb_query_transformer.py

22a5f24

vercel bot deployed to Preview June 29, 2023 18:09 View deployment

fix query.py

fbc7642

vercel bot deployed to Preview June 29, 2023 19:06 View deployment

john-z-yang marked this pull request as ready for review June 29, 2023 23:08

john-z-yang requested a review from a team as a code owner June 29, 2023 23:08

john-z-yang requested a review from a team June 29, 2023 23:08

john-z-yang requested review from a team as code owners June 29, 2023 23:08

iambriccardo reviewed Jun 30, 2023

View reviewed changes

iambriccardo approved these changes Jul 5, 2023

View reviewed changes

iambriccardo requested a review from RaduW July 5, 2023 14:22

RaduW suggested changes Jul 5, 2023

View reviewed changes

Merge branch 'master' into john/remove-uck

62941ea

vercel bot deployed to Preview July 5, 2023 17:40 View deployment

fix, wrong parameter name

7a8b3b6

vercel bot deployed to Preview July 6, 2023 11:05 View deployment

replace some more UseCaseKey with UseCaseID

fe2ddef

RaduW requested a review from a team July 6, 2023 16:18

vercel bot deployed to Preview July 6, 2023 16:20 View deployment

more missed UseCaseKey usages

fce1bf1

vercel bot deployed to Preview July 6, 2023 16:35 View deployment

john-z-yang requested a review from ayirr7 July 6, 2023 16:37

john-z-yang changed the title ~~ref(generic-metrics): Migrate UseCaseKey to UseCaseID~~ ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics Jul 6, 2023

Merge branch 'master' into john/remove-uck

bf993a1

vercel bot deployed to Preview July 7, 2023 07:30 View deployment

RaduW requested review from iambriccardo and obostjancic July 7, 2023 07:30

obostjancic reviewed Jul 10, 2023

View reviewed changes

src/sentry/api/endpoints/organization_metrics.py Show resolved Hide resolved

src/sentry/testutils/cases.py Show resolved Hide resolved

iambriccardo reviewed Jul 10, 2023

View reviewed changes

obostjancic approved these changes Jul 10, 2023

View reviewed changes

RaduW self-requested a review July 10, 2023 11:14

RaduW self-assigned this Jul 10, 2023

RaduW approved these changes Jul 10, 2023

View reviewed changes

RaduW merged commit 7dcae76 into master Jul 10, 2023
54 checks passed

RaduW deleted the john/remove-uck branch July 10, 2023 11:15

github-actions bot locked and limited conversation to collaborators Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics #51776

ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics #51776

john-z-yang commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading

iambriccardo left a comment

iambriccardo left a comment

RaduW left a comment •

edited

Loading

RaduW Jul 5, 2023

john-z-yang Jul 6, 2023 •

edited

Loading

RaduW Jul 6, 2023

john-z-yang Jul 6, 2023

RaduW Jul 5, 2023

iambriccardo Jul 10, 2023

RaduW Jul 5, 2023

RaduW Jul 5, 2023 •

edited

Loading

john-z-yang commented Jul 6, 2023 •

edited

Loading

RaduW commented Jul 6, 2023

iambriccardo left a comment

iambriccardo Jul 10, 2023

RaduW Jul 10, 2023

iambriccardo Jul 10, 2023

RaduW Jul 10, 2023

iambriccardo Jul 10, 2023

obostjancic left a comment

strings it accept	enum it maps to
`"performance"`	`UseCaseID.TRANSACTIONS`
`"transactions"`	`UseCaseID.TRANSACTIONS`
`"sessions"`	`UseCaseID.SESSIONS`
`"release-health"`	`UseCaseID.SESSIONS`

ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics #51776

ref(generic-metrics): Migrate UseCaseKey to UseCaseID for organization metrics #51776

Conversation

john-z-yang commented Jun 28, 2023 • edited Loading

Refactoring usages of UseCaseKey into UseCaseID

codecov bot commented Jun 28, 2023 • edited Loading

Codecov Report

iambriccardo left a comment

Choose a reason for hiding this comment

iambriccardo left a comment

Choose a reason for hiding this comment

RaduW left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-z-yang Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaduW Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

john-z-yang commented Jul 6, 2023 • edited Loading

RaduW commented Jul 6, 2023

iambriccardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obostjancic left a comment

Choose a reason for hiding this comment

john-z-yang commented Jun 28, 2023 •

edited

Loading

Refactoring usages of `UseCaseKey` into `UseCaseID`

codecov bot commented Jun 28, 2023 •

edited

Loading

RaduW left a comment •

edited

Loading

john-z-yang Jul 6, 2023 •

edited

Loading

RaduW Jul 5, 2023 •

edited

Loading

john-z-yang commented Jul 6, 2023 •

edited

Loading