-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add FID deprecation warn on web vital measure #5422
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
base: master
Are you sure you want to change the base?
Conversation
2f0630d to
41ae3a1
Compare
|
|
||
| type browserBuildFunc func(ctx, vuCtx context.Context) (*common.Browser, error) | ||
|
|
||
| var fidDeprecationWarningOnce sync.Once |
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.
I'm not entirely happy with this approach. Is there something I'm missing, maybe a state that I can rely on that allows me to log a single warning log regardless of how many iterations, VUs and scenarios are in the test?
d5a3723 to
1079c97
Compare
| fidDeprecationWarningOnce.Do(func() { | ||
| vu.State().Logger.Warnf("FID has been deprecated and superseded by INP -- https://web.dev/blog/fid") | ||
| }) | ||
|
|
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.
This seems like it will ALWAYS log this instead of only when this is used. I am not aware how this is used, but if it is going to always be logged and we will just drop it ... maybe just drop a warning that will be useless for most people and lets just drop it in v2 ?
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.
What about logging a warning when the FID metric is about to be emitted?
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.
Wouldn't that still happen each time someone uses browser - irregardless of what they are using it for /
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, i see what you're saying. The user could be relying on the metric in a threshold, what about adding a log when a threshold uses the metric? Any other ideas?
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.
as a quick POC, placing a warn log line here produces this:
> k6 run e2e/quickpizza-admin-user.js
WARN[0000] browser_web_vital_fid has been deprecated and superseded by browser_web_vital_inp
/\ Grafana /‾‾/
/\ / \ |\ __ / /
/ \/ \ | |/ / / ‾‾\
/ \ | ( | (‾) |
/ __________ \ |_|\_\ \_____/ So pretty hidden, whereas i would want the log line to be more visible. I also don't want to spend too long on this with lots of changes just to get the log line in the correct place, since this is temporary and FID will be removed in v2.
c3bf882 to
f9e449b
Compare
What?
When the FID metric is measured, a warning will be logged to let users know to move away from working with
browser_web_vital_fidand instead usebrowser_web_vital_inp.Why?
FID has been deprecated -- ttps://web.dev/blog/fid.
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
#5179