Add content processs histograms to the main ping payload

RESOLVED DUPLICATE of bug 1218576

Status

()

P3
normal
RESOLVED DUPLICATE of bug 1218576
2 years ago
2 years ago

People

(Reporter: gfritzsche, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected)

Details

(Whiteboard: [measurement:client])

(Reporter)

Description

2 years ago
With bug 1218576 in place, we need to put the histogram data from the child processes in the "main" ping payload.
(Reporter)

Comment 1

2 years ago
Chris raised the point that we probably have more child processes coming up (compositor per bug 1264543, possibly sandboxed addons, etc.), so that we should probably use a map structure to accommodate that.

E.g. one possible format:

> "payload": {
>  ...
>  "childHistograms": {
>    "content": {
>      ... histograms
>    },
>    "compositor: {
>      ... histograms
>    },
>  },
>  "childKeyedHistograms": {
>    ... similar
>  },
>  "childScalars": { // coming later, not now.
>    ...

Alernatively:
> "payload": {
>   ...
>   "childData": {
>     "histograms": {
>       "content": {
>         ...
>       },
>       ...
>     },
>     "keyedHistograms": {
>       ...
>     },
>     "scalars": { // Later, not now.
>       ...

I'd love to have the "main"/"parent" process histograms in there too, but i think that is not feasible due to the pipeline and analysis code depending on payload.histograms/keyedHistograms.
(Reporter)

Comment 2

2 years ago
Opinions on comment 1?
Flags: needinfo?(rvitillo)
Flags: needinfo?(mreid)
Flags: needinfo?(chutten)

Comment 3

2 years ago
I'd like to suggest a third possibility:
> "payload": {
>   ...
>   "processes": {
>     "content": {
>       "histograms": {...},
>       "keyedHistograms": {...},
>       "scalars": {...}
>     },
>     "compositor": {
>       "histograms": {...},
>       "keyedHistograms": {...},
>       "scalars": {...}
>     },
>     ... eventually, "parent" too? ...,
>     ...
>   }
>   ...
> }

This has a couple of main advantages.

Each section has the same structure, so ideally the processing code can be simple/reusable. 

Also, it allows for the possibility to move standard "parent" measurements into the same structure in the future if we decide it's worthwhile (it doesn't assume "child" for things).

If we wanted to introduce the "parent" section right away, we could even put scalars there for starters.
Flags: needinfo?(mreid)

Comment 4

2 years ago
I second mreid's suggestion. It's a little bit nested, but it's the clearest in intent.

I wonder if special processes will need special structures beyond the shared hgrams/keyed/scalars triumvirate (threadHangs? slowSQL?)
Flags: needinfo?(chutten)
+1 on Mark's proposal.
Flags: needinfo?(rvitillo)
(Reporter)

Comment 6

2 years ago
Great, lets go with this then.
We should definitely start off with scalars in "parent" right away (per bug 1282091).
(Reporter)

Comment 7

2 years ago
The new format approach also opens up possibilities for other use-cases:
E.g. for GoFaster / system addons / etc. we could add additional sections that have the same format but possibly different handling/semantics server-side.
(Reporter)

Updated

2 years ago
Blocks: 1277504
(Reporter)

Updated

2 years ago
Summary: Add content histograms to the main ping payload → Add content processs histograms to the main ping payload
(Reporter)

Comment 8

2 years ago
This is happening as part of bug 1218576 now.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1218576
You need to log in before you can comment on or make changes to this bug.