-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
ref(ddm): Dual write to Sentry metrics as well as Datadog #5474
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1655ef9
ref(ddm): Dual write to Sentry metrics as well as Datadog
evanh 4614315
fix tests
evanh 26408be
Merge branch 'master' into evanh/fix/use-sentry-metrics
evanh befa668
typing fixes
evanh f87cac7
fix typing/tests
evanh bea4cba
Merge branch 'master' into evanh/fix/use-sentry-metrics
evanh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
from __future__ import annotations | ||
|
||
import random | ||
|
||
from snuba import settings, state | ||
from snuba.utils.metrics.backends.abstract import MetricsBackend | ||
from snuba.utils.metrics.backends.datadog import DatadogMetricsBackend | ||
from snuba.utils.metrics.backends.sentry import SentryMetricsBackend | ||
from snuba.utils.metrics.types import Tags | ||
|
||
|
||
class SentryDatadogMetricsBackend(MetricsBackend): | ||
""" | ||
A metrics backend that records metrics to Sentry and Datadog. | ||
""" | ||
|
||
def __init__( | ||
self, datadog: DatadogMetricsBackend, sentry: SentryMetricsBackend | ||
) -> None: | ||
self.datadog = datadog | ||
self.sentry = sentry | ||
|
||
def _use_sentry(self) -> bool: | ||
if state.get_config("use_sentry_metrics", "0") == "1": | ||
return bool(random.random() < settings.DDM_METRICS_SAMPLE_RATE) | ||
return False | ||
|
||
def increment( | ||
self, | ||
name: str, | ||
value: int | float = 1, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
self.datadog.increment(name, value, tags, unit) | ||
if self._use_sentry(): | ||
self.sentry.increment(name, value, tags, unit) | ||
|
||
def gauge( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
self.datadog.gauge(name, value, tags, unit) | ||
if self._use_sentry(): | ||
self.sentry.gauge(name, value, tags, unit) | ||
|
||
def timing( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
self.datadog.timing(name, value, tags, unit) | ||
if self._use_sentry(): | ||
self.sentry.timing(name, value, tags, unit) | ||
|
||
def distribution( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
self.datadog.distribution(name, value, tags, unit) | ||
if self._use_sentry(): | ||
self.sentry.distribution(name, value, tags, unit) | ||
|
||
def events( | ||
self, | ||
title: str, | ||
text: str, | ||
alert_type: str, | ||
priority: str, | ||
tags: Tags | None = None, | ||
) -> None: | ||
self.datadog.events(title, text, alert_type, priority, tags) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
from __future__ import annotations | ||
|
||
from sentry_sdk import metrics | ||
|
||
from snuba.utils.metrics.backends.abstract import MetricsBackend | ||
from snuba.utils.metrics.types import Tags | ||
|
||
|
||
class SentryMetricsBackend(MetricsBackend): | ||
""" | ||
A metrics backend that records metrics to Sentry. | ||
""" | ||
|
||
def __init__(self) -> None: | ||
return None # Sentry doesn't require any setup | ||
|
||
def increment( | ||
self, | ||
name: str, | ||
value: int | float = 1, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
metrics.incr(name, value, unit or "none", tags) | ||
|
||
def gauge( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
metrics.gauge(name, value, unit or "none", tags) | ||
|
||
def timing( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
# The Sentry SDK has strict typing on the unit, so it doesn't allow passing arbitrary units | ||
metrics.timing(name, value, unit or "millisecond", tags) # type: ignore | ||
|
||
def distribution( | ||
self, | ||
name: str, | ||
value: int | float, | ||
tags: Tags | None = None, | ||
unit: str | None = None, | ||
) -> None: | ||
metrics.distribution(name, value, unit or "none", tags) | ||
|
||
def events( | ||
self, | ||
title: str, | ||
text: str, | ||
alert_type: str, | ||
priority: str, | ||
tags: Tags | None = None, | ||
) -> None: | ||
return None # Sentry doesn't support events |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timings are distributions -- i don't think we need both. the implementation in the SDK is identical
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not identical, because in the SDK they accept different units. The timing ones specifically only accept timing units, and distributions are more flexible.
So we should be using
distribution
when we are logging things that aren't timing based. See https://github.com/getsentry/snuba/blob/master/snuba/clickhouse/http.py#L253 for an example that should be changed todistribution
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have a default impl for timing that uses distribution internally or the other way around? The impl really is identical in the SDK (the only thing that differs is type hints and return value) -- i get that you want to have different defaults for unit in each case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not both? Both exist in the Sentry SDK and in Datadog. Why not use the SDKs as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because in both SDKs they're literally just aliases and now you're forcing every backend to implement those aliases.
the purpose of
sentry_sdk.metrics.timing
is to provide a decorator and context manager to measure a codeblock -- we're not using thatThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, but we are missing the
distribution
functionality from all our backends, so I would have to add that no matter what (timing is a subset of distribution). Why not leave timing in place, add distributions, and then our backends line up with both of the production SDKs (DD and Sentry).If the aliases are already available, why not leverage them? Why in turn make timing another alias on distribution? The DD and Sentry SDKs already provide that alias for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you really don't, you could continue to use timing or rename it to distribution if you want to. timing is not a subset of distribution in master branch, it's just distribution with maybe an unfortunate (not-technically-correct) name. the semantic difference was created by adding unit to the interface. i don't see the point in adding more methods that do the same thing but only differ in their default for unit, but I don't feel too strongly about all of this so i'm approving