misc(proto): LighthouseConfig #7614
Conversation
There was a problem hiding this comment.
I'm assuming this is a required part of the smokehouse in LR deal? Seems great to me if only we can use it for now, but I have some concerns about exposing this to the world if that's what will happen.
Not 100% clear on how careful we need to be with proto changes or if there are separate internal CLs that are responsible for the API docs and whatnot :)
| map<string, string> extra_headers = 15; | ||
|
|
||
| // Precomputed lantern estimates to use instead of observed analysis. | ||
| PrecomputedLanternData precomputed_lantern_data = 16; |
There was a problem hiding this comment.
I'm not super excited about exposing this in its current form to the world, is everything here going to be available over the PSI API automatically?
There was a problem hiding this comment.
All these changes will only be exposed in LR atm. But perhaps in the future we'd expand PSI to allow a config. We could disallow whatever settings we want from PSI.
Do you have a refactor of this setting in mind? If so, we should do that before encoding in proto. If this is just a setting that's only useful for testing I don't think the shape matters much.
There was a problem hiding this comment.
Do you have a refactor of this setting in mind?
Well in my variance doc I proposed removing it completely depending on what direction we want to head, so I wouldn't want a brand new setting to popup for PSI users only to take it away a few weeks later :)
There was a problem hiding this comment.
We could leave it out completely (from the proto) + disable the smokeTest that uses in Smokerider indefinitely.
| @@ -138,10 +149,49 @@ message LighthouseResult { | |||
|
|
|||
| // List of the categories that were run, empty if all were run | |||
| // nullable list of strings | |||
| // Note: this should have been google.protobuf.ListValue, but too late now. | |||
There was a problem hiding this comment.
What impact does this have on us? (same with onlyAudits I assume?
There was a problem hiding this comment.
Little. The python / go proto serializers handle both just fine, given a list of strings. Technically LR users can pass anything to only_categories (like a string or a struct), and it would pass the serialization with flying colors, but ultimately fail the request (or at least, the setting would get ignored. i have not tried it). But the same is true for ListValue (pass a list of int32s, no bueno). so it's already an existing shortcoming.
There was a problem hiding this comment.
I'm assuming this is a required part of the smokehouse in LR deal? Seems great to me if only we can use it for now, but I have some concerns about exposing this to the world if that's what will happen.
It'll only be exposed via LR's request api. No plans for PSI. Whether it lands in the internal part of the request or part of the "public" api, doesn't change anything on LH's side. But we can discuss it here now. I'd argue against keeping it private - why would we want to limit LR users?
Not 100% clear on how careful we need to be with proto changes or if there are separate internal CLs that are responsible for the API docs and whatnot :)
I suppose internal tools like bequt will update automatically (as you'd expect for the service that is defined with a proto), but I think you mean like PSI's docs? afaik that won't change b/c something in LR changes.
| map<string, string> extra_headers = 15; | ||
|
|
||
| // Precomputed lantern estimates to use instead of observed analysis. | ||
| PrecomputedLanternData precomputed_lantern_data = 16; |
There was a problem hiding this comment.
All these changes will only be exposed in LR atm. But perhaps in the future we'd expand PSI to allow a config. We could disallow whatever settings we want from PSI.
Do you have a refactor of this setting in mind? If so, we should do that before encoding in proto. If this is just a setting that's only useful for testing I don't think the shape matters much.
| @@ -138,10 +149,49 @@ message LighthouseResult { | |||
|
|
|||
| // List of the categories that were run, empty if all were run | |||
| // nullable list of strings | |||
| // Note: this should have been google.protobuf.ListValue, but too late now. | |||
There was a problem hiding this comment.
Little. The python / go proto serializers handle both just fine, given a list of strings. Technically LR users can pass anything to only_categories (like a string or a struct), and it would pass the serialization with flying colors, but ultimately fail the request (or at least, the setting would get ignored. i have not tried it). But the same is true for ListValue (pass a list of int32s, no bueno). so it's already an existing shortcoming.
It's mostly |
|
@patrickhulce we can cut the lantern stuff, you ok with losing w/e coverage the lantern-expectations would get us in LR? |
I'm fine either way :) You've reconvinced me that it would be worthwhile to have this property in LR contexts, and waiting to expose it also wouldn't hurt anything, so I'm happy whichever you decide 👍 |
|
We decided to spend more time and thought on the config before going all-in on the proto. I can do a hacky thing for Smokerider's needs for now. Parking things in the branch |
Summary
Just the proto changes. See #7613 for more.
Related Issues/PRs
#7613