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): Add generic metrics interface #46957

Merged
merged 29 commits into from
Apr 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
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
64 changes: 64 additions & 0 deletions src/sentry/sentry_metrics/metrics_interface.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
from abc import ABC, abstractmethod
from typing import Mapping, Optional, Sequence, Union

from sentry.sentry_metrics.use_case_id_registry import UseCaseID


class GenericMetricsBackend(ABC):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this interface is going to produce on Kafka it will be asynchronous. Still you will likely want to allow the client to wait for the message to be acknowledged (callback). How do you plan to support this use case?

You will likely want an interface that allows you to swap the backend (relay or kafka) but still allow delivery guarantee. How will it look like in relay ? Will the call be synchronous ?

There are multiple ways to achieve this:

  • have a sync api and an async api
  • have only an async api and simulate the async behavior in relay with a background thread that wait for the HTTP response.

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 think, to start, maybe we can have an aysnc api and a sync api. I would start working on this while building the Kafka backend, which will be the next PR. Is there anything else specific to this PR that would need to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should figure out how the async interface looks like (how the client receives the callback, is that a future?) at this point as you will need such interface immediately for Kafka.

Copy link
Member Author

@ayirr7 ayirr7 Apr 11, 2023

Choose a reason for hiding this comment

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

Yes, I am thinking about that for the Kafka implementation. Since this PR implements the UseCaseID though, it's a bit blocking for some other work. How would it be if we merged the current state of the interface and, in another PR, revisit it as needed + build the Kafka implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separate from Filippo's comment, I think on the base class we should have docstrings for each method.

@abstractmethod
def counter(
self,
use_case_id: UseCaseID,
org_id: int,
project_id: int,
metric_name: str,
value: Union[int, float],
unit: Optional[str],
tags: Mapping[str, str],
) -> None:

"""
Used for emitting a counter metric for internal use cases only.
Ensure that the use_case_id passed in has been registered
in the UseCaseID enum.
"""

raise NotImplementedError()

@abstractmethod
def set(
self,
use_case_id: UseCaseID,
org_id: int,
project_id: int,
metric_name: str,
value: Sequence[int],
unit: Optional[str],
tags: Mapping[str, str],
) -> None:

"""
Used for emitting a set metric for internal use cases only. Can support
a sequence of values. Ensure that the use_case_id passed in has
been registered in the UseCaseID enum.
"""
raise NotImplementedError()

@abstractmethod
def distribution(
self,
use_case_id: UseCaseID,
org_id: int,
project_id: int,
metric_name: str,
value: Sequence[Union[int, float]],
unit: Optional[str],
tags: Mapping[str, str],
) -> None:

"""
Used for emitting a distribution metric for internal use cases only. Can
support a sequence of values. Ensure that the use_case_id passed in
has been registered in the UseCaseID enum.
"""
raise NotImplementedError()
22 changes: 22 additions & 0 deletions src/sentry/sentry_metrics/use_case_id_registry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from __future__ import annotations

from enum import Enum
from typing import Mapping

from sentry.sentry_metrics.configuration import UseCaseKey


class UseCaseID(Enum):
TRANSACTIONS = "transactions"
SESSIONS = "sessions"


# UseCaseKey will be renamed to MetricPathKey
METRIC_PATH_MAPPING: Mapping[UseCaseID, UseCaseKey] = {
UseCaseID.SESSIONS: UseCaseKey.RELEASE_HEALTH,
UseCaseID.TRANSACTIONS: UseCaseKey.PERFORMANCE,
}


john-z-yang marked this conversation as resolved.
Show resolved Hide resolved
def get_metric_path_from_usecase(use_case: UseCaseID) -> UseCaseKey:
return METRIC_PATH_MAPPING[use_case]