Fix performance custom metrics

[tunetheweb]

Noticed performance custom metrics were not getting logged in our CI tests.

This was because the current logic was as follows:

          try {
            wpt_custom_metrics[`_${metric_name}`] = JSON.parse(wpt_custom_metric);
            if (metrics_to_log.includes(metric_name)) {
              wpt_custom_metrics_to_log[`_${metric_name}`] = JSON.parse(wpt_custom_metric);
            }

          } catch (e) {
            wpt_custom_metrics[`_${metric_name}`] = wpt_custom_metric;
          }

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 the if (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:

• https://example.com/
• https://exploreti.com/

[max-ostapenko]

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?

[tunetheweb]

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: #113 (comment)

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.

[tunetheweb]

Updated PR to add the comment to explain why.

[github-actions]

Custom metrics for https://almanac.httparchive.org/en/2022/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240619_7D_4
Changed custom metrics values:

{}

Custom metrics for https://example.com/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240619_17_5
Changed custom metrics values:

{}

Custom metrics for https://exploreti.com/

WPT test run results: http://webpagetest.httparchive.org/results.php?test=240619_09_6
Changed custom metrics values:

{}

[max-ostapenko]

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.

@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).

[tunetheweb]

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.

[max-ostapenko]

I think this PR makes these tests more similar to the events processing logic.

[tunetheweb]

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 😀

[max-ostapenko]

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.

[tunetheweb]

Oh very similar! Nice find. OK merging this.