Lighthouse: Remove rawValue from Audit.Result

[exterkamp]

What is the current behavior?

rawValue is still in each Audit in the LHR, but the Metrics audit contains all rawValue information and so rawValue is redundant.

[paulirish]

The big change here is asking people to pull metric values from audits.metrics.details instead of audits[metricId].rawValue

I'm cool with that, though.

[benschwarz]

I might've misunderstood this issue a little bit, but on a cursory look, dom-size, time-to-first-byte and bootup-time are not contained within the metrics audit.

[paulirish]

• dom-size: yes we dont have a great answer for this. we changed it for formatting recently and now we broke the usecase of grabbing these values
• time-to-first-byte: hmmmm yes we dont expose a value here that'd be useful. .details.overallSavingsMs is something else.
• bootup-time: same # available via .details.summary.wastedMs.

[benschwarz]

I guess for dom-size we can add a .details.summary.value so that it's similar to bootup-time.

[exterkamp]

Started to work on this with branch.

Currently I have removed rawValue from auditResult objects so that the value is never surfaced to the LHR. But it is still used internally in the auditProduct objects.

[brendankenny]

But it is still used internally in the auditProduct objects.

Since (I believe) this isn't blocking the proto work, I think we should do this right and eliminate the rawValue from the product as well. Otherwise it will live (and rot) forever for no benefit but being able to ignore it today.

This might be one of those cases we all need to get in a single (virtual) room and make sure every useful rawValue has a reasonable details that handles it

[exterkamp]

@brendankenny :+1: I agree. The current branch _works_ but obviously isn't clean and will rawValue will definitely rot forever. I will continue to make the changes to auditResult.

[connorjclark]

Since we delete rawValue for LR, that presents problems with smoke tests that assert on it. But I'll rectify that by swapping over to assertions on score. (EDIT: no that's hard).

Are we leaning towards deleting rawValue entirely?

EDIT: actually, there's only a clean transition from rawValue => score assertions for binary values. The redirects audit has extendedInfo.value, which dupes rawValue. Is the idea to remove rawValue in favor of extendedInfo?

[connorjclark]

via LR, the robots-txt audit doesn't provide a sane way to check for success (since rawValue is dropped, and we provide no score value). Just another example of how our scoring story could be improved.

// OK
{
  "description": "If your robots.txt file is malformed, crawlers may not be able to understand how you want your website to be crawled or indexed.",
  "id": "robots-txt",
  "score": null,
  "scoreDisplayMode": "notApplicable",
  "title": "robots.txt is valid"
}

// not OK
{
  "description": "If your robots.txt file is malformed, crawlers may not be able to understand how you want your website to be crawled or indexed.",
  "id": "robots-txt",
  "score": null,
  "scoreDisplayMode": "notApplicable",
  "title": "robots.txt is not valid"
}

[patrickhulce]

@Hoten maybe a regex on title /is valid/?

Is the idea to remove rawValue in favor of extendedInfo?

The idea was to remove rawValue in favor of a special metrics details timing, but I'm not sure we got around to making that a reality

[connorjclark]

@Hoten maybe a regex on title /is valid/?

I didn't mean to suggest it's impossible, just that there's not a generic way to check if a particular audit passed. font-size is just one, they're are probably others?

Some other examples show that rawValue is pretty useful for testing purposes. Many smoke tests assert on it, and there's no nice alternative to rawValue. For example:

1. time-to-first-byte (see perf/expectations.js) uses rawValue to assert on time in ms until first byte.
2. interactive and first-cpu-idle (see tricky-metrics/expectations.js) uses rawValue. The alternative is to assert on the "log normal" score, which would make the actual assertion unclear.

[patrickhulce]

Gotcha my bad :)

yeah we will likely have to finally flesh out details to fully replace rawValue finally to make this happen then

[connorjclark]

I perhaps should not have kicked this thread, as it seems we let this idea die =P

I would like that we keep rawValues in the audit results. LR removes them by default, but I can easily add a flag that prevents that behavior (and enable it just for running the smoke tests).

If some longer term endeavor begins to kill rawValue, I think that'd be a detriment to the smoke tests.

[brendankenny]

we definitely need to kill rawValue. It came from a time when we had like three audits and thought "we don't know what we're going to need, let's just put whatever needs to be output in rawValue".

We just punted on it because it wasn't anyone's job and we wanted to get 4.0 out the door. displayValue or details should be able to cover everything we need, we just need to make them actually used.

[exterkamp]

Kicking this again for #7752. I think our needs are something like this:

• For smoketests we need some pass fail indicator + a numeric indicator.
• For diagnostics we would like some numeric summary of the audit that is easier than parsing the display value or title (e.g. TTFB in ms as an int)

In order to accomplish those in both LH and LR, I think we should introduce a top-level variable instead of rawValue, something like numericValue. And score can be an indicator for pass/fail audits.

Note: since we never added rawValue to the proto, we can add it now, so the name can stay the same, we can just strictly type it as an int in LH and proto and ts are happy!

[brendankenny]

I think this is definitely doable for v5. And we don't really know the timing for v6 (it could be as late as May 2020 depending on whatever), so if we want to do it now's the time.

We will break more than a few folks digging into the LHR for actual metric values, but I think it's worth it. rawValue is vaguely named and what it should contain is poorly defined, so a change will only make things more usable.

The general plan we came up with was:

1) right now rawValue is the only required property on the audit return value. Switch to make score the only required property.
2) add score to all the audits that don't explicitly set it and remove rawValue where it's null or a boolean. For the most part this will be s/rawValue/score in those audits
3) where rawValue is numeric and useful to have, make it optional, rename it to numericValue (or whatever self-explanatoryish name we come up with), and give it type number.

The change should be fairly trivial since for the most part where rawValue will need to be removed it's already being used for its automatic copy to score anyways. It'll just be "trivial" * 130 (or however many audits we have now) + tests.

[upugo-dev]

Hey @brendankenny , this is a fairly breaking change for us. We were using rawValue's boolean value to determine audit pass/fail status, or if it was a numerical value, some custom threshold to then get a true/false from it.

To be clear, if we are writing custom audits that are boolean in nature, what property should we return on the audit to indicate this? And how should we be testing existing (lighthouse core/default) audits for their boolean status?

For example, it looks like here https://github.com/GoogleChrome/lighthouse/blob/7750fe465d818a5bfd191ce7d959d82a2052eb8c/lighthouse-core/audits/is-on-https.js#L71 you are returning a 0 or 1 for the score, corresponding to a t/f whether there are any items or not, yeah? Which is the result of the "is this website on https" (?) Is the score always between 0 and 1?

[patrickhulce]

you are returning a 0 or 1 for the score, corresponding to a t/f whether there are any items or not, yeah?

Correct, score === 1 is the new rawValue === true for boolean audits :)

Is the score always between 0 and 1?

Correct, score is guaranteed to be between 0 and 1. scoreDisplayMode typically indicates if we intend the user to treat the score as a boolean or number. See understanding results doc for details.

[brendankenny]

In addition to what @patrickhulce said,

Hey @brendankenny , this is a fairly breaking change for us. We were using rawValue's boolean value to determine audit pass/fail status, or if it was a numerical value, some custom threshold to then get a true/false from it.

Yes, sorry about that. We tried to call it out prominently in the release notes, but maybe a migration guide would have been a good idea.

As I outlined in https://github.com/GoogleChrome/lighthouse/pull/8421#issuecomment-484790725, rawValue was pretty much historical accumulation and its ambiguity was making each of its possible use cases less useful. In order to have raw metric values in the audit results and to keep the property from becoming unfocused over time again we narrowed the concept down to the more explicit numericValue.

We could make a similar thing for the boolean-ish audits, but as @patrickhulce notes, scoreDisplayMode: 'binary' and the score already handle this case, so it's probably not worth adding a redundant property.

(it was also never inherently clear what most of those audits should be returning for a 'rawValue' in the first place. e.g. for is-on-https, should rawValue have been the number of requests not on https or just a boolean state because (as browsers treat a page load) either all requests were secure or they weren't? Both possible values are useful. Instead we have explicit places for both of those numbers: boolean goes to the 0/1 score, the number of insecure requests in details.items).

[upugo-dev]

@patrickhulce @brendankenny awesome guys, thanks for your comments as always!