Closed tunetheweb closed 4 months ago
Thanks @tunetheweb
Do you know why some metrics are wrapped into JSON.stringify and others not? Would anything change in BQ if we drop JSON.stringify?
Thanks @tunetheweb
Do you know why some metrics are wrapped into JSON.stringify and others not? Would anything change in BQ if we drop JSON.stringify?
Some of the custom metrics call JSON.stringify()
in the return (example for media custom-metric) and some don't (example for performance custom-metric which just returns the object directly).
Mostly it doesn't matter, and Web Page Test will handle both and stringify()
automatically when it needs to store them (e.g. when saving to it's Database).
However, one place it CAN matter is if some results aren't included in JSON.stringify()
(mostly due to incorrect code!). If you're running the code in the console to test it you can miss that if you don't stringify it and think it's working and then be surprised when it doesn't work in WebPageTest. So best to do this explicitly IMHO so you see the same failures in the console. See long conversation on this here: https://github.com/HTTPArchive/custom-metrics/pull/113#issuecomment-2043937823
Probably we should standardise this and update all the custom metrics to JSON.stringify()
the final return
so we don't have this inconsistency. But in meantime this works.
Updated PR to add the comment to explain why.
Probably we should standardise this and update all the custom metrics to
JSON.stringify()
the finalreturn
so we don't have this inconsistency. But in meantime this works.
@tunetheweb so maybe we rather wrap performance CM (and other metrics that have the same issue) in JSON.stringify
?
It will give us more confidence regarding further metrics parsing in the data processing pipeline (as you mentioned in the linked comment).
Yeah we probably should. But I think we'd still want to keep this change in anyway incase a future metric was added without it, or some of the non-JSON custom metrics can also return "invalid JSON" (and converting those to JSON is a bigger job with bigger implications).
Plus it's more efficient (calls JSON.parse
only once).
So not super urgent IMHO once we merge this.
I think this PR makes these tests more similar to the events processing logic.
I think this PR makes these tests more similar to the events processing logic.
Not sure what you mean? And if you are saying that was a good thing or a bad thing 😀
Not sure what you mean? And if you are saying that was a good thing or a bad thing 😀
It's good. Here is actual pipeline code which is similar to your update.
Oh very similar! Nice find. OK merging this.
Noticed performance custom metrics were not getting logged in our CI tests.
This was because the current logic was as follows:
That says only log metrics we're interested in if
JSON.parse
succeeds.However, many metrics are returned as Objects rather than JSON and so
JSON.parse
fails (as it doesn't need to be parsed as it'a already an object!), and others aren't valid JSON as just plain strings. In both cases theif (metrics_to_log.includes(metric_name)) {
part isn't called and they are excluded from the output.This PR refactors to move code out of the
try
to log in both cases.Test websites: