core: replace rawValue with numericValue in audit result #8385

brendankenny · 2019-04-18T01:07:38Z

part 2 of apparently 3 for #6199

Following #8343, this now renames the public rawValue on LH.Audit.Result to numericValue. We have very few places we rely on this except the smoke tests, so it's an easy change and a better place to bikeshed on numericValue and the public interface than the next PR :)

The last PR will be renaming the variable internal to audits/audit tests.

brendankenny · 2019-04-18T01:09:11Z

lighthouse-core/lib/proto-preprocessor.js

-      // Drop raw values. https://github.com/GoogleChrome/lighthouse/issues/6199
-      if (!keepRawValues && 'rawValue' in audit) {
-        delete audit.rawValue;
+      // Drop numeric values until we decide what to do with the optional type in proto.


punting on the question of DoubleValue/Value/whatever in proto by maintaining the deleting by default behavior of rawValue. We can change it to whatever later.

I think it is going to have to be Value but this is something that we can punt 🏈

(replying to this thread)

On the other hand.. this seems a great time to not punt on the proto typing for this prop. :) All our PSI API users currently don't get rawValue because we're punting and thus have to parse strings like "3,241ms" in order to get some metric values.

.numericValue being typed as the mega-generic proto Value seems real bad.

What are the challenges to typing .numericValue as DoubleValue?

Maybe I misunderstood but we are punting on adding the property at all to the proto right, not proposing to start with it as Value?

Afaik DoubleValue doesn't support a difference between 'null' or 'undefined' and '0' since it is the default. I can do some more testing around that, but thats the crux. BUT if we don't care about undefined = 0, then we can for sure use a more typical and better proto type!

Hold up - DoubleValue is nullable. double and float are the ones that can't distinguish between 0 and not provided.

Maybe I misunderstood but we are punting on adding the property at all to the proto right, not proposing to start with it as Value?

yeah, that's what I was proposing, not changing the status quo on proto, but we can do this for real

@exterkamp and I just tested a few possibilities and, yeah, DoubleValue will work well for us. 0 comes through, and then any audit with it not set will come back with no numericValue in JSON, which works just fine for us.

brendankenny · 2019-04-18T01:09:53Z

lighthouse-core/lib/proto-preprocessor.js

-        delete audit.rawValue;
+      // Drop numeric values until we decide what to do with the optional type in proto.
+      // https://github.com/GoogleChrome/lighthouse/issues/6199
+      if (!keepRawValues && 'numericValue' in audit) {


not sure how flexible we are in changing the keepRawValues parameter name in the lr entry point, but happy to rename if it's not a big deal

we can def change it.

we can def change it.

do you want me to change it now and LR will just have to remember to update on the next roll?

well, if we are happy with numericValue now shouldn't we just always include it. remove the option entirely.

well, if we are happy with numericValue now shouldn't we just always include it. remove the option entirely.

removed :)

brendankenny · 2019-04-18T01:10:05Z

lighthouse-core/test/lib/manifest-parser-test.js

@@ -347,7 +347,6 @@ describe('Manifest Parser', function() {
      const display = parsedManifest.value.display;
      assert.ok(!display.warning);
      assert.equal(display.value, 'browser');
-      assert.equal(display.rawValue, undefined);


this made no sense, not a thing

patrickhulce · 2019-04-18T01:53:35Z

I'm a little unclear on what the purpose of this PR is, are we planning on exposing an audit-level property called numericValue/whatever we decide on that has the numbers here and not the details.timingMs route or whatever?

If we are, then I think I was confused about what killing rawValue meant since this doesn't seem fundamentally different :)

If we aren't and this is all temporary, then I have absolutely no preference on the name so merge away 😆

brendankenny · 2019-04-18T05:17:24Z

are we planning on exposing an audit-level property called numericValue/whatever we decide on that has the numbers here and not the details.timingMs route or whatever?

That's what's this PR does but it's not necessarily the plan :)

I don't think we ever actually settled on a plan, so this is bikeshedding on property name and object structure. Fun for everyone!

I'll try to make a better case tomorrow, but basically I think a large portion of our programmatic users just need something like this, a single numeric value that's a companion to the score, and don't want (or need) to dig into details to do it. They're also not all ms numbers, so it's hard to name the value. I also realize that this doesn't really differentiate the property from rawValue except for the (already removed) primacy and booleaness of the property in the audit product :)

brendankenny · 2019-04-18T05:19:03Z

(I am curious how many of these remaining rawValues are actually already covered by things in their details...)

benschwarz · 2019-04-18T06:17:16Z

I think a large portion of our programmatic users just need something like this, a single numeric value that's a companion to the score, and don't want (or need) to dig into details to do it.

👬 Strong upvote from me on that @brendankenny

exterkamp · 2019-04-18T06:30:57Z

a single numeric value that's a companion to the score

This is the concept. While I've been trying to analyze variance this has come up a lot. RawValue is not suitable since it is non-numeric and has a lot of legacy baggage. So we wanted a numeric only value that could be used for analysis. e.g. TTFB score: 0.7, numericValue: 1159 (1159ms) | DOM size score: 0.9, numericValue: 14 (14 nodes).

exterkamp · 2019-04-18T06:36:32Z

lighthouse-core/lib/proto-preprocessor.js

-      // Drop raw values. https://github.com/GoogleChrome/lighthouse/issues/6199
-      if (!keepRawValues && 'rawValue' in audit) {
-        delete audit.rawValue;
+      // Drop numeric values until we decide what to do with the optional type in proto.


I think it is going to have to be Value but this is something that we can punt 🏈

docs/understanding-results.md

patrickhulce · 2019-04-18T15:17:34Z

I think a large portion of our programmatic users just need something like this, a single numeric value that's a companion to the score, and don't want (or need) to dig into details to do it.

As one of those programmatic users I also strongly upvote this :)

SGTM!

paulirish · 2019-04-18T16:12:07Z

lighthouse-core/lib/proto-preprocessor.js

-      // Drop raw values. https://github.com/GoogleChrome/lighthouse/issues/6199
-      if (!keepRawValues && 'rawValue' in audit) {
-        delete audit.rawValue;
+      // Drop numeric values until we decide what to do with the optional type in proto.


(replying to this thread)

On the other hand.. this seems a great time to not punt on the proto typing for this prop. :) All our PSI API users currently don't get rawValue because we're punting and thus have to parse strings like "3,241ms" in order to get some metric values.

.numericValue being typed as the mega-generic proto Value seems real bad.

What are the challenges to typing .numericValue as DoubleValue?

patrickhulce

numericValue as a name has my 🙏 👍

should be minimal effort to migrate too :D

patrickhulce · 2019-04-18T16:30:28Z

lighthouse-core/test/scoring-test.js

@@ -35,7 +35,7 @@ describe('ReportScoring', () => {
  describe('#scoreAllCategories', () => {
    it('should score the categories', () => {
      const resultsByAuditId = {
-        'my-audit': {rawValue: 'you passed', score: 0},
+        'my-audit': {score: 0},
        'my-boolean-audit': {score: 1, extendedInfo: {}},


man, never thought I'd see the day where extendedInfo outlives rawValue 😆

man, never thought I'd see the day where extendedInfo outlives rawValue 😆

hahaha, maybe it's time to do the final purge on that

core: replace rawValue with numericValue in audit result

5e42ce5

brendankenny requested review from patrickhulce and paulirish as code owners April 18, 2019 01:07

paulirish mentioned this pull request Apr 18, 2019

☂️ 👣 Breaking changes for v5 #7752

Closed

8 tasks

brendankenny commented Apr 18, 2019

View reviewed changes

exterkamp reviewed Apr 18, 2019

View reviewed changes

paulirish requested changes Apr 18, 2019

View reviewed changes

patrickhulce reviewed Apr 18, 2019

View reviewed changes

proto numeric_value, remove keepRawValues

444db06

vercel bot deployed to staging April 18, 2019 20:19 View deployment

paulirish approved these changes Apr 18, 2019

View reviewed changes

brendankenny merged commit f2bd3b6 into master Apr 19, 2019

brendankenny deleted the rawvalue branch April 19, 2019 00:40

brendankenny mentioned this pull request Apr 19, 2019

core: replace remaining rawValue with numericValue #8421

Merged

paulirish mentioned this pull request Apr 30, 2019

[meta] 2019 I/O Burndown #8331

Closed

67 tasks

brendankenny mentioned this pull request May 29, 2020

report: handle non-numeric numericValues in calc link #10880

Merged

AndrewBarber mentioned this pull request Jan 24, 2021

Replaced rawValue to numericValue in Lighthouse NPM Script bigcommerce/cornerstone#1970

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: replace rawValue with numericValue in audit result #8385

core: replace rawValue with numericValue in audit result #8385

brendankenny commented Apr 18, 2019

brendankenny Apr 18, 2019

exterkamp Apr 18, 2019

paulirish Apr 18, 2019 •

edited

Loading

patrickhulce Apr 18, 2019

exterkamp Apr 18, 2019

connorjclark Apr 18, 2019

brendankenny Apr 18, 2019

brendankenny Apr 18, 2019

brendankenny Apr 18, 2019

paulirish Apr 18, 2019

brendankenny Apr 18, 2019

connorjclark Apr 18, 2019

brendankenny Apr 18, 2019

brendankenny Apr 18, 2019

patrickhulce commented Apr 18, 2019 •

edited

Loading

brendankenny commented Apr 18, 2019

brendankenny commented Apr 18, 2019

benschwarz commented Apr 18, 2019

exterkamp commented Apr 18, 2019

exterkamp Apr 18, 2019

patrickhulce commented Apr 18, 2019

paulirish Apr 18, 2019 •

edited

Loading

patrickhulce left a comment

patrickhulce Apr 18, 2019

brendankenny Apr 18, 2019

core: replace rawValue with numericValue in audit result #8385

core: replace rawValue with numericValue in audit result #8385

Conversation

brendankenny commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 18, 2019 • edited Loading

brendankenny commented Apr 18, 2019

brendankenny commented Apr 18, 2019

benschwarz commented Apr 18, 2019

exterkamp commented Apr 18, 2019

Choose a reason for hiding this comment

patrickhulce commented Apr 18, 2019

paulirish Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Apr 18, 2019 •

edited

Loading

patrickhulce commented Apr 18, 2019 •

edited

Loading

paulirish Apr 18, 2019 •

edited

Loading