-
Notifications
You must be signed in to change notification settings - Fork 108
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
enabled web-vitals-extension loggin option #80
Conversation
This is a great feature! Reduces the anxiety of having to click the extension at the right time without doing anything else that might interfere with what you wanted to measure! |
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.
Looks great, thanks @gilbertococchi! One comment.
src/browser_action/vitals.js
Outdated
@@ -210,12 +219,21 @@ | |||
// of animations or highly-dynamic content, we | |||
// debounce the broadcast of the metric. | |||
latestCLS = metric; | |||
if(enableLogging) { |
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.
Should each of these metrics' logs be moved to broadcastMetricsUpdates
? That will also debounce the CLS logging to prevent console spam.
One other suggestion is to prefix the log message with something identifiable for filtering, eg "[Web Vitals] "
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.
Hi @rviscomi , thanks for your feedback.
Yes I agree with you, at first I've added the logging within broadcastMetricsUpdates
but then I've moved to the getMetricName method because I wanted to create one line per Metric logging.
I've moved the logging to that method, I thought about keeping the metric value rounded toFixed(2) in the first logging value and send as last log entry the whole metric object.
Maybe we can support different logging levels, such as debug (metric name + value) and verbose (metric name, value, metric object with all the entries).
WDYT?
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.
Maybe we can support different logging levels, such as debug (metric name + value) and verbose (metric name, value, metric object with all the entries).
WDYT?
I think the way you have it here is a good first step for this PR and we can keep #77 open to revisit it with enhancements like the one you suggested.
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.
LGTM!
I'll defer to @addyosmani to give this one last look before merging.
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.
LGTM. Thank you!
We'll include this in our next release.
Progress on #77
Added Console Log Options to enable all CWV Metrics logging via console.