feat(metrics): Add trace metrics behind an experiments flag #4898
Conversation
Similar to getsentry/sentry-javascript#17883, this allows the py sdk to send in new trace metric protocol items, although this code is experimental since the schema may still change. Most of this code has been copied from logs (eg. log batcher -> metrics batcher) in order to dogfood, once we're more sure of our approach we can refactor.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
k-fish
changed the title
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| def _metric_to_transport_format(metric): | ||
| # type: (TraceMetric) -> Any | ||
| def format_attribute(val): | ||
| # type: (int | float | str | bool) -> Any |
There was a problem hiding this comment.
Potential bug: The type hint int | float | str | bool in _trace_metrics_batcher.py is incompatible with supported Python versions older than 3.10, causing a SyntaxError on import.
-
Description: The type annotation for the
format_attributefunction uses theint | float | str | boolunion syntax. This syntax was introduced in Python 3.10. However, the package'ssetup.pydeclares support for Python versions 3.6 and newer. As a result, importing the_trace_metrics_batchermodule on Python versions 3.6 through 3.9 will raise an immediateSyntaxError, preventing the application from starting and causing the trace metrics feature to be completely non-functional on these supported versions. -
Suggested fix: Replace the
int | float | str | boolsyntax withUnion[int, float, str, bool]from thetypingmodule. This will ensure the type hint is compatible with all supported Python versions from 3.6 onwards.
severity: 0.9, confidence: 1.0
Did we get this right? 👍 / 👎 to inform future reviews.
…ce trace metric is an internal dataset detail
| from sentry_sdk.api import get_client, get_current_scope, get_current_span | ||
| from sentry_sdk.utils import safe_repr |
There was a problem hiding this comment.
The imports should be at the top unless there's a good reason not to have them there.
See my other comment regarding avoiding circular imports.
| @@ -1,4 +1,5 @@ | |||
| from sentry_sdk import profiler | |||
| from sentry_sdk import trace_metrics | |||
There was a problem hiding this comment.
We should try to avoid circular imports. See the other comment I left. We also don't re-export many other modules. Can we remove the re-export?
To my knowledge the only downside is that it wouldn't be included in from sentry_sdk import *. If there's a good reason to keep it we should still try to avoid the circular import when importing at the top of a file.
Note that most other modules are not re-exported here.
### Description Remove old metrics code to make way for #4898 Metrics was always an experimental feature and Sentry stopped accepting metrics a year ago. #### Issues <!-- * resolves: #1234 * resolves: LIN-1234 --> #### Reminders - Please add tests to validate your changes, and lint your code using `tox -e linters`. - Add GH Issue ID _&_ Linear ID (if applicable) - PR title should use [conventional commit](https://develop.sentry.dev/engineering-practices/commit-messages/#type) style (`feat:`, `fix:`, `ref:`, `meta:`) - For external contributors: [CONTRIBUTING.md](https://github.com/getsentry/sentry-python/blob/master/CONTRIBUTING.md), [Sentry SDK development docs](https://develop.sentry.dev/sdk/), [Discord community](https://discord.gg/Ww9hbqr)
Summary
Similar to getsentry/sentry-javascript#17883, this allows the py sdk to send in new trace metric protocol items, although this code is experimental since the schema may still change. Most of this code has been copied from logs (eg. log batcher -> metrics batcher) in order to dogfood, once we're more sure of our approach we can refactor.
Closes LOGS-367