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.

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

Get rid of protobufs in P3A pings #15967

Closed
iefremov opened this issue May 19, 2021 · 12 comments · Fixed by brave/brave-core#11600
Closed

Get rid of protobufs in P3A pings #15967

iefremov opened this issue May 19, 2021 · 12 comments · Fixed by brave/brave-core#11600

Comments

@iefremov
Copy link
Contributor

For now, a P3A pings consists of a base64-encoded protobuf.

message RawP3AValue {
  fixed64 metric_id = 1;
  bytes p3a_info = 2;
}

given that pings are small we can get rid of protobuf and replace it with simple json.

{
  "metric_id" : "hex-encoded-metric-id-string",
  "p3a_info" : "p3a info string" 
}

It is suggested to keep p3a string as is, because the current plan is to resurrect prochlo soon, presumbaly prochlo will utilize this format (of everything packed into p3a_info) somehow.

@iefremov iefremov added OS/Android Fixes related to Android browser functionality OS/Desktop labels May 19, 2021
@iefremov iefremov self-assigned this May 19, 2021
@iefremov
Copy link
Contributor Author

cc @aekeus @porteron

@porteron
Copy link
Member

This will be great for us on the stats side. @iefremov once you have data writing in the new format to the bucket let us know so we can update our aggregator to no longer attempt to decode the data.

@iefremov
Copy link
Contributor Author

since we removed PROCHLO, nothing stops us from killing the "ugly string" (aka p3a_info) and making a normal json. I'll update the description with more details.

@iefremov
Copy link
Contributor Author

I'm not sure about release notes for this change, @mattmcalister what do you think?

@mattmcalister
Copy link

@iefremov just say what it is... Migrate ping data to json format or something like that

@iefremov
Copy link
Contributor Author

@mattmcalister I mean, I'm not sure whether this is release-notes/include or /exclude

@mattmcalister
Copy link

@rebron ...up to you.

@LaurenWags
Copy link
Member

@iefremov @rillian since this is QA/Yes, could we get a test plan for this one? Thanks! cc @GeetaSarvadnya

@rillian
Copy link

rillian commented Feb 3, 2022

I've added a brief test plan to brave/brave-core#11600.

Sorry about that, and please let me know if you need more details.

@LaurenWags
Copy link
Member

thanks @rillian! I'll defer to @GeetaSarvadnya to determine if more info is needed. This looks like a good starting point 👍🏻

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Feb 8, 2022

Verification PASSED on


Brave | 1.36.87 Chromium: 98.0.4758.87 (Official Build) beta (64-bit)
-- | --
Revision | e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS | Windows 10 Version 21H2 (Build 19044.1503)

json format json format
image (5) image (4)
  • Waited for ~15 mins and confirmed count of json transmitted values (28 metrics) are matching with the list of values/stats in brave://local-state under p3a.logs (28 metrics)
brave://local-state json format transmitted values
image image (4)
  • Waited for ~50 mins and confirmed count of json transmitted values (33 metrics) are matching with the list of values/stats in brave://local-state under p3a.logs (33 metrics)
brave://local-state json format transmitted values
image (4) image (5)
  • Confirmed packets to the https://p3a.brave.com/ endpoint is similar to what was sent before the change, encoded as a binary protocol buffer.
encoded binary encoded binary
image (4) image (5)

Verification PASSED using

Brave 1.36.88 Chromium: 98.0.4758.87 (Official Build) dev (x86_64)
Revision e4cd00f135fb4d8edc64c8aa6ecbe7cc79ebb3b2-refs/branch-heads/4758@{#1002}
OS macOS Version 11.6.1 (Build 20G224)
json format json format
Screen Shot 2022-02-08 at 3 27 58 PM Screen Shot 2022-02-08 at 3 30 56 PM

NOTE: unable to confirm count of JSON-transmitted values (36 metrics) matches the list of values/stats in brave://local-state under p3a.logs (42 metrics), however, there are 6 p2a.brave.com entries, which might accommodate it.

brave://local-state JSON-format transmitted values p2a.brave.com
Screen Shot 2022-02-08 at 3 43 21 PM Screen Shot 2022-02-08 at 3 43 58 PM Screen Shot 2022-02-08 at 3 55 52 PM
  • Confirmed p3a.brave.com and p3a-json.brave.com endpoints have the same ping count (separate run from other verifications).
p3a.brave.com p3a-json.brave.com
Screen Shot 2022-02-08 at 10 03 56 PM Screen Shot 2022-02-08 at 10 04 05 PM
  • Confirmed packets to the https://p3a.brave.com/ endpoint is similar to what was sent before the change, encoded as a binary protocol buffer.
encoded binary encoded binary
Screen Shot 2022-02-08 at 3 54 24 PM Screen Shot 2022-02-08 at 3 54 29 PM
  • Confirmed packets to the p2a.brave.com endpoint are similar to what was sent before the change, encoded as a binary protocol buffer.
encoded binary encoded binary
Screen Shot 2022-02-08 at 3 51 57 PM Screen Shot 2022-02-08 at 3 52 08 PM

Verification PASSED using

Brave 1.36.107 Chromium: 99.0.4844.45 (Official Build) (64-bit)
Revision edbc0b8343c7b10fddb0e1b4efb280b0f6e38cab-refs/branch-heads/4844@{#788}
OS Ubuntu 18.04 LTS

image

  • Confirmed p3a.brave.com and p3a-json.brave.com endpoints have the same ping count

image

  • Confirmed packets to the https://p3a.brave.com/ endpoint is similar to what was sent before the change, encoded as a binary protocol buffer.

image

  • Confirmed packets to the p2a.brave.com endpoint are similar to what was sent before the change, encoded as a binary protocol buffer.

image

@Uni-verse
Copy link
Contributor

Uni-verse commented Mar 1, 2022

Verification Completed using 1.36.108 Chromium 99.0.4844.51
OS: Android ARM
Device: Samsung GS 21
Firmware: Android 12

PASSED - Confirm packets to the https://p3a-json.brave.com/ endpoint are in [json format]
PASSED - Transmitted values should match what's listed in brave://local-state under p3a.logs.
PASSED -Confirmed p3a.brave.com and p3a-json.brave.com endpoints have the same ping count
PASSED - Confirm packets to the https://p3a.brave.com/ and https://p2a.brave.com/ endpoints are similar to what sent before the change, encoded as a binary protocol buffer, instead of in a human-readable strings like the json endpoint receives.

Screen Shot 2022-03-01 at 2 29 26 PM
Screen Shot 2022-03-01 at 2 29 03 PM

rillian added a commit to brave/brave-core that referenced this issue Jul 29, 2022
Now that we only have one message format, replace the parallel
log stores with a single one.

Follow-up to brave/brave-browser#15967
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment