Refactor function names and logic in Metrics #14459
Draft
Conversation
This file contains hidden or 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
cemms1
commented
Sep 2, 2025
|
|
||
| const sampling = 1 / 100; | ||
| /** defining this here allows to share this with other metrics */ | ||
| const willRecordCoreWebVitals = Math.random() < sampling; |
There was a problem hiding this comment.
This is used for both CWV and commercial metrics so better to have a value that covers both. Additionally, this is not the only reason the metrics might be recorded; this simply represents whether the page view is in the sample or not
cemms1
commented
Sep 2, 2025
| if (isUndefined(pageViewId)) return; | ||
|
|
||
| const bypassSampling = shouldBypassSampling(abTestApi); | ||
| const recordMetrics = isInSample || shouldBypassSampling(abTestApi); |
There was a problem hiding this comment.
The logic determining whether metrics will be recorded is:
- is the page view in the sample?
- should the sampling be bypassed? ie should metrics be collected for all page views for a particular scenario?
|
"This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days" |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does this change?
Renames
bypassSamplingtorecordMetricsas this better represents what this value means.Introduces another value
isInSampleto replacewillRecordCoreWebVitalssince this is used for both commercial metrics and core web vitals, and represents whether the request is in the sample or not.Additionally takes this value
isInSampleout of thebypassSamplingdecision seeing as these are two distinct things - in order to record metrics, you should either be in the sample or if not, the sampling should be bypassed in order to record all metricsWhy?
Making the code more easy to understand for future developers