Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Proto file missing fields #15193

Open
MordFustang21 opened this issue Jun 21, 2023 · 5 comments · May be fixed by #15616
Open

Proto file missing fields #15193

MordFustang21 opened this issue Jun 21, 2023 · 5 comments · May be fixed by #15616
Assignees
@paulirish
Labels
P1

Comments

@MordFustang21
Copy link

Summary

We're using the proto definition defined here but when we look at the output of the JSON the following extra fields are added which breaks our unmarshal into types generated by the proto.

message ConfigSettings {
  string output = 10;
  int64 max_wait_for_fcp = 11;
  int64 max_wait_for_load = 12;
  int64 pause_after_fcp_ms = 13;
  int64 pause_after_load_ms = 14;
  int64 network_quiet_threshold_ms = 15;
  int64 cpu_quiet_threshold_ms = 16;
  string emulated_user_agent = 17;
  bool audit_mode = 18;
  bool gather_mode = 19;
  bool disable_storage_reset = 20;
  bool debug_navigation = 21;
  bool use_passive_gathering = 22;
  bool disable_full_page_screenshot = 23;
  bool skip_about_blank = 24;
  string blank_page = 25;
  repeated string budgets = 26;
  repeated string blocked_url_patterns = 27;
  repeated string additional_trace_categories = 28;
  repeated string extra_headers = 29;
  repeated string precomputed_lantern_data = 30;
  repeated string only_audits = 31;
  repeated string skip_audits = 32;
}

CC @derekperkins
@paulirish
Copy link
Member

Oh nice. Yeah we know we've been leaving this out, but it wasnt causing problems for us.
(And we didnt expect anyone else to use the proto definition).

The above resolves things on your side? We might opt for double instead of int64. At first glance the rest look good tho.

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 22, 2023 •
edited
Loading

I feel like int is the right call for these fields. Sub ms resolution is kinda of pointless for these things, right?

@paulirish
Copy link
Member

I feel like int is the right call for these fields. Sub ms resolution is kinda of pointless for these things, right?

Yes I agree. We just don't have any ints so far. And I kinda suspect they might cause a problem for us somewhere, so I don't want to commit to it just yet :)

@paulirish paulirish added the P1 label Aug 22, 2023
@adamraine adamraine assigned paulirish and unassigned connorjclark Nov 14, 2023
paulirish added a commit that referenced this issue Nov 16, 2023
@paulirish
proto: add ConfigSettings fields. thx #15193
98d5cb6
@paulirish
Copy link
Member

paulirish commented Nov 16, 2023 •
edited
Loading

Yes I agree. We just don't have any ints so far. And I kinda suspect they might cause a problem for us somewhere, so I don't want to commit to it just yet :)

turns out int64 serializes to json as a string (at least in the python package). but int32 goes to a number. so we good.

@derekperkins
Copy link
Contributor

I think int64 as json strings is pretty universal for safety. Not sure if that's in the spec, but that's what I see in practice

@paulirish paulirish linked a pull request Nov 16, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@paulirish paulirish

Labels
P1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

misc(proto): add ConfigSettings fields GoogleChrome/lighthouse
6 participants
@paulirish @derekperkins @connorjclark @MordFustang21 @devtools-bot and others