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

Expanding Built-In Performance Metrics for Browser JavaScript #3

Merged
merged 7 commits into from Sep 21, 2022

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Aug 18, 2022

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


# 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`.

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.

Copy link
Member Author

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.

  1. Convert connection.rtt and connection.downlink into built-in measurements. The value here is obv, and we are already collecting this data -> monitor the effect for 1 week
  2. Add hardware.concurrency -> monitor the effect for 1 week
  3. 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.

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

@mitsuhiko mitsuhiko changed the title 0003 - Expanding Built-In Performance Metrics for Browser JavaScript Expanding Built-In Performance Metrics for Browser JavaScript Aug 21, 2022
@danielkhan
Copy link

@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.
But even if we don't show them today, what could be achieved by showing them?
E.g. long rtt on a fast downlink might indicate a problem with global distribution.

TL;DR: Can we provide OOTB features on top of metrics like these? \cc @smeubank

@AbhiPrasad
Copy link
Member Author

AbhiPrasad commented Aug 29, 2022

Thanks @danielkhan - great point on the value prop. The connection.X one's are relatively straightforward - they help signify to users that network strategies might need to be improved (PoP introduction, more aggressive caching, etc.). There is an argument here that though that only one is needed, perhaps just knowing connection.rtt is enough to generally understand network conditions.

Upon doing further research here though I've discovered that the value of connection.downlink can never exceed 10Mbps (https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation/downlink#browser_compatibility). This is done for fingerprinting reasons. Same with connection.rtt, it can never exceed 3000 ms. Perhaps the value there is limiting.

For device.memory, it seems that WebKit won't ever support this. It's imprecise OOB for fingerprinting reasons, but at a high level still valuable because it can help developer analyze if most of their users are using high/low powered devices.

Finally, there is hardware.concurrency is valuable since it can help users decide on usage of APIs like Web Workers, but perhaps it's better suited in processor_count under Device Context.

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:

  1. connection.rtt - used to analyze network quality at a high level
  2. device.memory - used to understand device power

The third is new:

  1. inp - Interaction to Next Paint, the newest chrome web vital.

We'll be adding support for tracking inp when we upgrade to web vitals v3, and then we eventually start tracking user interactions - so it would be good to get this in here and approved before that.

hardware.concurrency will be put into device context instead, under the processor_count field, and connection.downlink will be removed as it's redundant with connection.rtt.

@danielkhan
Copy link

@AbhiPrasad - so this means that some metrics can be useful.

  • Do we expose them today in any way?
  • How do we expect the user to find them?
  • How would a user story look like for this?

I would want to avoid just adding more data without surfacing it or making it actionable to the user.

@AbhiPrasad AbhiPrasad self-assigned this Sep 7, 2022
@AbhiPrasad
Copy link
Member Author

This RFC has been updated to only propose two new built in metrics for browser JS, connection.rtt and inp. It is now ready to review.

@AbhiPrasad AbhiPrasad marked this pull request as ready for review September 21, 2022 14:30
@AbhiPrasad
Copy link
Member Author

There's some work already to add inp as a built-in metric, so this is slightly retroactive:

Copy link
Member

@k-fish k-fish left a 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.

@AbhiPrasad
Copy link
Member Author

Merging this in as a) inp work is in progress b) the team has come to a decision about rtt

@AbhiPrasad AbhiPrasad merged commit 5aef64d into main Sep 21, 2022
@AbhiPrasad AbhiPrasad deleted the abhi-built-in-measurements branch September 21, 2022 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants