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

Remove the web vital rate metric time series metrics #915

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

ankur22
Copy link
Collaborator

@ankur22 ankur22 commented May 31, 2023

Description of changes

There are multiple reasons for making this change:

  1. We're reducing the number of time series.
  2. The rating should live as close to the value that it represents.
  3. The rating in the summary is noise that doesn't add value unless we break them down by url.
  4. This shouldn't increase the cardinality of the metric that we're labelling with the rating. It's a better trade off than having more time series metrics.
  5. The summary at the end will be a bit more focused and shorter.

Test

You should be able to run any example test (e.g. the fillform.js test). The web vital summary output should now look like this (note that we no longer have the rating metrics):

     browser_web_vital_cls..............: avg=0.000029 min=0        med=0.000029 max=0.000057 p(90)=0.000052 p(95)=0.000055
     browser_web_vital_fcp..............: avg=288.9ms  min=131.79ms med=240ms    max=494.9ms  p(90)=443.92ms p(95)=469.41ms
     browser_web_vital_fid..............: avg=500µs    min=100µs    med=500µs    max=899.99µs p(90)=819.99µs p(95)=859.99µs
     browser_web_vital_inp..............: avg=136ms    min=16ms     med=136ms    max=256ms    p(90)=232ms    p(95)=244ms   
     browser_web_vital_lcp..............: avg=313.35ms min=131.79ms med=313.35ms max=494.9ms  p(90)=458.59ms p(95)=476.74ms
     browser_web_vital_ttfb.............: avg=219.13ms min=105.09ms med=212.29ms max=340ms    p(90)=314.45ms p(95)=327.22ms

If you run it against influxdb or another backend you should find the new rating label like so:

Screenshot 2023-05-31 at 17 43 56

Checklist

Tasks

There are multiple reasons for making the change:

1. We're reducing the number of time series.
2. The rating should live as close to the value that it represents.
3. The rating in the summary is noise that doesn't add value unless we
break them down by url.
4. This shouldn't increase the cardinality of the metric that we're
labelling with the rate. It's a better trade off than having more
time series metrics.
5. The summary at the end will be a bit more focused and shorter.
@ankur22 ankur22 added the breaking PRs that need to be mentioned in the breaking changes section of the release notes label May 31, 2023
@ankur22 ankur22 added this to the v0.10.0 milestone May 31, 2023
@ankur22 ankur22 requested review from inancgumus and ka3de May 31, 2023 11:23
@ankur22 ankur22 self-assigned this May 31, 2023
common/frame_session.go Outdated Show resolved Hide resolved
This matches what we already use elsewhere in the code.

Resolves: #915 (comment)
Copy link
Collaborator

@ka3de ka3de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@inancgumus inancgumus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking PRs that need to be mentioned in the breaking changes section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants