-
Notifications
You must be signed in to change notification settings - Fork 87
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(server): Track metrics for OpenTelemetry events #1618
Conversation
9d3f5a9
to
76d6f4a
Compare
76d6f4a
to
f84ec30
Compare
f84ec30
to
40f661d
Compare
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.
A couple of questions to better understand the intention, not blocking.
.and_then(Annotated::value); | ||
|
||
if otel_context.is_some() { | ||
metric!( |
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.
Collecting the metrics in finalize_event
means you'll get metrics for events relay drops as part of light normalization), is this your intention?
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.
Yes this was my intention, the same as transaction source metrics we are collecting.
This will be the estimate we use to understand the consequences of doing more complicated processing of opentelemetry events in Relay, details of which we will share with the Ingest team very soon.
.and_then(Annotated::value); | ||
|
||
if otel_context.is_some() { | ||
metric!( |
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.
How many OTel events (and metrics) are we expecting, and how big is the cardinality of these counters (are most events coming from the same SDK) are in the statsd client in relay? If the number is not large it's ok. My concern is that we've been in the maximum threshold before, and the additional metrics are dropped.
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.
We should be consuming at the same volume as transaction source information, which is set above in RelayCounters::EventTransactionSource
with the same cardinality, and we've been able to manage that fine.
Most events should come from 4 SDKs atm (Node, Java, Python, Ruby), so it's even less than transaction source at the moment.
* master: chore(toolchain): Fix lint and test for beta toolchain (#1623) ref(system): Introduce a generic recipient type (#1622) feat(transaction): Normalize transaction name (#1621) feat(server): Track metrics for OpenTelemetry events (#1618) bug(replays): Increase reserved space in payload chunks (#1601) feat(protocol): Add OpenTelemetry Context (#1617)
fixes #1615
This PR adds a statsd metric for OpenTelemetry events. It allows us to see at a high level the volume of OpenTelemetry and what SDKs they are being sent from.