-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Expanding Built-In Performance Metrics for Browser JavaScript #3
Conversation
2ed6c24
to
1ccd07e
Compare
de92e6c
to
2d2aba0
Compare
8c9766c
to
3b1b367
Compare
|
||
# Summary | ||
|
||
This RFC details expanding list of built-in performance metrics for the browser JavaScript SDK, with additional data the Browser SDK captures. It propose adding two metrics that are already captured by the SDK: `connection.rtt` and `connection.downlink`, and three brand new metrics that are not yet captured, `device.memory`, `hardware.concurrency`, and `long_task.count`. |
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.
Every new metric we extract increases the volume we need to ingest on the metrics pipeline and increases the storage size very quickly as suddenly all sdks start sending them for every transactions as soon as they are upgraded.
Is there a way we can phase such a rollout ?
Right now we extract an average of 10 metrics per transaction. If we add 5 that represents 50%. Considering that javascript is one of the major sdks we have this will be a substantial increase.
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.
So 2 of these connection.rtt
, and connection.downlink
are already being sent by the SDK - but they are considered custom metrics instead of built-in metrics.
I agree though that we should consider phasing such a rollout, great suggestion. Ok so let's do this.
- Convert
connection.rtt
andconnection.downlink
into built-in measurements. The value here is obv, and we are already collecting this data -> monitor the effect for 1 week - Add
hardware.concurrency
-> monitor the effect for 1 week - Add
device.memory
-> monitor the effect for 1 week
Based on some discussion with performance team and @k-fish we are dropping long_task.count
from the proposal, it's not that valuable as we already collect the spans.
If we consider the effect not worth it, we can drop device.memory
, but I feel we should not drop hardware.concurrency
.
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.
Ok,
also, something important to keep in mind.
If you add a standard tag to metrics or change the name of one (basically anything that is not custom), please register it on this file and statically assign it an ID.
https://github.com/getsentry/sentry/blob/master/src/sentry/sentry_metrics/indexer/strings.py.
So the indexer will not assign a new ID to that tag key for every single organization and they all will share the same ID
@AbhiPrasad what I am missing in this discussion is which value these two metrics provide to the user. Do we show them anywhere? I assume not, as they are not part of the built-in metrics right now. TL;DR: Can we provide OOTB features on top of metrics like these? \cc @smeubank |
Thanks @danielkhan - great point on the value prop. The Upon doing further research here though I've discovered that the value of For Finally, there is I also got some other additional feedback, I'm going to adjust this proposal to add three new built metrics instead: Two of them are from the existing proposal:
The third is new:
We'll be adding support for tracking
|
@AbhiPrasad - so this means that some metrics can be useful.
I would want to avoid just adding more data without surfacing it or making it actionable to the user. |
This RFC has been updated to only propose two new built in metrics for browser JS, |
There's some work already to add |
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 definitely want to add INP
as a web vital (and have been talking about it already for a while). rtt
is also helpful for diagnosing latency related slowness, and we've been sending it for a long time. In my opinion both are useful to us, so 👍.
As for surfacing it, we'll be making some changes to highlight INP in our product.
Merging this in as a) |
This RFC details expanding list of built-in performance metrics for the browser JavaScript SDK, with additional data the Browser SDK captures. It propose adding a metric that is already captured by the SDK:
connection.rtt
and a brand new metric that is not yet captured,inp
.Rendered RFC