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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 17 commits into from
Jul 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
38da146
implement use case id/key compatibility in metrics
john-z-yang Jun 28, 2023
3e2179c
implement use case id/key compatibility in org_metrics
john-z-yang Jun 28, 2023
1c40763
fix tests
john-z-yang Jun 28, 2023
74cb79e
Revert "fix tests"
john-z-yang Jun 28, 2023
37aaa27
fix tests
john-z-yang Jun 29, 2023
2616cca
update cases.py
john-z-yang Jun 29, 2023
caae53f
update test
john-z-yang Jun 29, 2023
86157c7
fix get_series callsites
john-z-yang Jun 29, 2023
22a5f24
fix mqb_query_transformer.py
john-z-yang Jun 29, 2023
fbc7642
fix query.py
john-z-yang Jun 29, 2023
2a4e56d
use get_use_case_key to resolve metric path
john-z-yang Jun 30, 2023
d723e53
compare with UseCaseID only
john-z-yang Jul 3, 2023
62941ea
Merge branch 'master' into john/remove-uck
john-z-yang Jul 5, 2023
7a8b3b6
fix, wrong parameter name
RaduW Jul 6, 2023
fe2ddef
replace some more UseCaseKey with UseCaseID
RaduW Jul 6, 2023
fce1bf1
more missed UseCaseKey usages
RaduW Jul 6, 2023
bf993a1
Merge branch 'master' into john/remove-uck
RaduW Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/sentry/api/endpoints/organization_measurements_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from sentry.api.bases import NoProjects, OrganizationEventsEndpointBase
from sentry.models import Organization
from sentry.search.events.constants import METRIC_FUNCTION_LIST_BY_TYPE
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import UseCaseID
from sentry.snuba.metrics.datasource import get_custom_measurements


Expand All @@ -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=UseCaseID.TRANSACTIONS,
)

with start_span(op="transform", description="metric meta"):
Expand Down
10 changes: 5 additions & 5 deletions src/sentry/api/endpoints/organization_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from sentry.api.exceptions import ResourceDoesNotExist
from sentry.api.paginator import GenericOffsetPaginator
from sentry.api.utils import InvalidParams
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import UseCaseID
from sentry.snuba.metrics import (
QueryDefinition,
get_metrics,
Expand All @@ -21,17 +21,17 @@
from sentry.utils.cursors import Cursor, CursorResult


def get_use_case_id(request: Request) -> UseCaseKey:
def get_use_case_id(request: Request) -> UseCaseID:
obostjancic marked this conversation as resolved.
Show resolved Hide resolved
"""
Get useCase from query params and validate it against UseCaseKey enum type
Get useCase from query params and validate it against UseCaseID enum type
Raise a ParseError if the use_case parameter is invalid.
"""

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]}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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]}"
        )```

Copy link
Member Author

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks!



Expand Down
4 changes: 2 additions & 2 deletions src/sentry/release_health/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
)
from sentry.release_health.metrics_sessions_v2 import run_sessions_query
from sentry.sentry_metrics import indexer
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import UseCaseID
from sentry.snuba.metrics import (
MetricField,
MetricGroupByField,
Expand Down Expand Up @@ -75,7 +75,7 @@
HOUR = MINUTE * 60
DAY = HOUR * 24
LEGACY_SESSIONS_DEFAULT_ROLLUP = HOUR
USE_CASE_ID = UseCaseKey.RELEASE_HEALTH
USE_CASE_ID = UseCaseID.SESSIONS

logger = logging.getLogger(__name__)

Expand Down
4 changes: 2 additions & 2 deletions src/sentry/release_health/metrics_sessions_v2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
SessionsQueryResult,
SessionsQueryValue,
)
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import UseCaseID
from sentry.snuba.metrics import get_public_name_from_mri
from sentry.snuba.metrics.datasource import get_series
from sentry.snuba.metrics.naming_layer import SessionMRI
Expand Down Expand Up @@ -570,7 +570,7 @@ def run_sessions_query(
metrics_results = get_series(
projects,
metrics_query,
use_case_id=UseCaseKey.RELEASE_HEALTH,
use_case_id=UseCaseID.SESSIONS,
tenant_ids={"organization_id": org_id},
)
except OrderByNotSupportedOverCompositeEntityException:
Expand Down
13 changes: 5 additions & 8 deletions src/sentry/search/events/builder/metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
WhereType,
)
from sentry.sentry_metrics import indexer
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import UseCaseID
from sentry.snuba.dataset import Dataset
from sentry.snuba.metrics.fields import histogram as metrics_histogram
Expand Down Expand Up @@ -652,9 +651,9 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any:
metrics_data = get_series(
projects=self.params.projects,
metrics_query=metric_query,
use_case_id=UseCaseKey.PERFORMANCE
use_case_id=UseCaseID.TRANSACTIONS
if self.is_performance
else UseCaseKey.RELEASE_HEALTH,
else UseCaseID.SESSIONS,
include_meta=True,
tenant_ids=self.tenant_ids,
)
Expand Down Expand Up @@ -841,9 +840,7 @@ def get_snql_query(self) -> Request:
metrics_query=transform_mqb_query_to_metrics_query(
snuba_request.query, is_alerts_query=self.is_alerts_query
),
use_case_id=UseCaseKey.PERFORMANCE
if self.is_performance
else UseCaseKey.RELEASE_HEALTH,
use_case_id=UseCaseID.TRANSACTIONS if self.is_performance else UseCaseID.SESSIONS,
).get_snuba_queries()

if len(snuba_queries) != 1:
Expand Down Expand Up @@ -1055,9 +1052,9 @@ def run_query(self, referrer: str, use_cache: bool = False) -> Any:
metrics_data = get_series(
projects=self.params.projects,
metrics_query=metric_query,
use_case_id=UseCaseKey.PERFORMANCE
use_case_id=UseCaseID.TRANSACTIONS
if self.is_performance
else UseCaseKey.RELEASE_HEALTH,
else UseCaseID.SESSIONS,
include_meta=True,
tenant_ids=self.tenant_ids,
)
Expand Down
53 changes: 40 additions & 13 deletions src/sentry/sentry_metrics/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
from sentry.api.utils import InvalidParams
from sentry.sentry_metrics import indexer
from sentry.sentry_metrics.configuration import UseCaseKey
from sentry.sentry_metrics.use_case_id_registry import (
METRIC_PATH_MAPPING,
REVERSE_METRIC_PATH_MAPPING,
UseCaseID,
)

#: Special integer used to represent a string missing from the indexer
STRING_NOT_FOUND = -1
Expand All @@ -15,9 +20,19 @@ class MetricIndexNotFound(InvalidParams):
pass


def coerce_use_case_key(use_case: Union[UseCaseID, UseCaseKey]) -> UseCaseID:
if isinstance(use_case, UseCaseKey):
return REVERSE_METRIC_PATH_MAPPING[use_case]
return use_case


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,
Comment on lines 29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

index: Union[int, str, None],
weak: bool = False,
) -> Optional[str]:
use_case_id = coerce_use_case_key(use_case_id)
if isinstance(index, str) or index is None:
return index
else:
Expand All @@ -27,8 +42,9 @@ def reverse_resolve_tag_value(
return reverse_resolve(use_case_id, org_id, index)


def reverse_resolve(use_case_id: UseCaseKey, org_id: int, index: int) -> str:
def reverse_resolve(use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, index: int) -> str:
assert index > 0
use_case_id = coerce_use_case_key(use_case_id)
resolved = indexer.reverse_resolve(use_case_id, org_id, index)
# The indexer should never return None for integers > 0:
if resolved is None:
Expand All @@ -37,52 +53,61 @@ def reverse_resolve(use_case_id: UseCaseKey, org_id: int, index: int) -> str:
return resolved


def reverse_resolve_weak(use_case_id: UseCaseKey, org_id: int, index: int) -> Optional[str]:
def reverse_resolve_weak(
use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, index: int
) -> Optional[str]:
"""
Resolve an index value back to a string, special-casing 0 to return None.

This is useful in situations where a `GROUP BY tags[123]` clause produces a
tuple for metric buckets that are missing that tag, i.e. `tags[123] == 0`.
"""

use_case_id = coerce_use_case_key(use_case_id)
if index == TAG_NOT_SET:
return None

return reverse_resolve(use_case_id, org_id, index)


def resolve(
use_case_id: UseCaseKey,
use_case_id: Union[UseCaseID, UseCaseKey],
org_id: int,
string: str,
) -> int:
use_case_id = coerce_use_case_key(use_case_id)
resolved = indexer.resolve(use_case_id, org_id, string)
if resolved is None:
raise MetricIndexNotFound(f"Unknown string: {string!r}")

return resolved


def resolve_tag_key(use_case_id: UseCaseKey, org_id: int, string: str) -> str:
def resolve_tag_key(use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, string: str) -> str:
use_case_id = coerce_use_case_key(use_case_id)
resolved = resolve(use_case_id, org_id, string)
assert use_case_id in (UseCaseKey.PERFORMANCE, UseCaseKey.RELEASE_HEALTH)
if use_case_id == UseCaseKey.PERFORMANCE:
assert isinstance(use_case_id, UseCaseID)
if METRIC_PATH_MAPPING[use_case_id] is UseCaseKey.PERFORMANCE:
return f"tags_raw[{resolved}]"
else:
return f"tags[{resolved}]"


def resolve_tag_value(use_case_id: UseCaseKey, org_id: int, string: str) -> Union[str, int]:
def resolve_tag_value(
use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, string: str
) -> Union[str, int]:
use_case_id = coerce_use_case_key(use_case_id)
assert isinstance(string, str)
assert use_case_id in (UseCaseKey.PERFORMANCE, UseCaseKey.RELEASE_HEALTH)
if use_case_id == UseCaseKey.PERFORMANCE:
assert isinstance(use_case_id, UseCaseID)
if METRIC_PATH_MAPPING[use_case_id] is UseCaseKey.PERFORMANCE:
return string
return resolve_weak(use_case_id, org_id, string)


def resolve_tag_values(
use_case_id: UseCaseKey, org_id: int, strings: Sequence[str]
use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, strings: Sequence[str]
) -> Sequence[Union[str, int]]:
use_case_id = coerce_use_case_key(use_case_id)
rv = []
for string in strings:
resolved = resolve_tag_value(use_case_id, org_id, string)
Expand All @@ -92,14 +117,15 @@ def resolve_tag_values(
return rv


def resolve_weak(use_case_id: UseCaseKey, org_id: int, string: str) -> int:
def resolve_weak(use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, string: str) -> int:
"""
A version of `resolve` that returns -1 for missing values.

When using `resolve_weak` to produce a WHERE-clause, it is quite
useful to make the WHERE-clause "impossible" with `WHERE x = -1` instead of
explicitly handling that exception.
"""
use_case_id = coerce_use_case_key(use_case_id)
resolved = indexer.resolve(use_case_id, org_id, string)
if resolved is None:
return STRING_NOT_FOUND
Expand All @@ -108,12 +134,13 @@ def resolve_weak(use_case_id: UseCaseKey, org_id: int, string: str) -> int:


def resolve_many_weak(
use_case_id: UseCaseKey, org_id: int, strings: Sequence[str]
use_case_id: Union[UseCaseID, UseCaseKey], org_id: int, strings: Sequence[str]
) -> Sequence[int]:
"""
Resolve multiple values at once, omitting missing ones. This is useful in
the same way as `resolve_weak` is, e.g. `WHERE x in values`.
"""
use_case_id = coerce_use_case_key(use_case_id)
rv = []
for string in strings:
resolved = resolve_weak(use_case_id, org_id, string)
Expand Down
Loading