Closed Bug 1590346 Opened 9 months ago Closed 8 months ago

Provide schema support for measurement units

Categories

(Tree Management :: Perfherder, task, P1)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: igoldan, Assigned: igoldan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Update performance-artifact-schema.json schema, so that value fields are accompanied by a new unit field of type string. I think a length of 24 characters suffices. Initially, treat this field as optional.

It should have a predefined list of values, like:

  • ms (milliseconds)
  • sec (seconds)
  • score
  • ratio
  • kbytes
  • bytes
  • count

This ticket is pretty dependent on bug 1584936. After bug 1584936 lands, we should wait for 1 day, until all available measurement units are ingested. That way, we can provide some extra predefined values to the upper list.

Priority: -- → P1

We've collected the measurement units which where already submitted. This is the list:

measurement unit total perf signatures
(NULL) 380,100
ms 65,039
score 34,600
1/FPS 4,488
ms/frame 594
% 208
frame interval 176
mWh 120
mAh 112
KB 45
Attachment #9103223 - Attachment description: Bug 1590346 - WIP Provide json schema support for measurement units → Bug 1590346 - Provide json schema support for measurement units

I'm wondering why there needs to be a predefined list of unit values? It seems like this is going to add effort/time for adding new measurements which don't fall into these fields and I don't see a benefit to this.

Flags: needinfo?(igoldan)
Status: NEW → ASSIGNED

(In reply to Greg Mierzwinski [:sparky] from comment #3)

I'm wondering why there needs to be a predefined list of unit values? It seems like this is going to add effort/time for adding new measurements which don't fall into these fields and I don't see a benefit to this.

The main problem I saw was the short form of the measurement units, which test owners can provide. It can be ambiguous or lack meaning.

The predefined list gives us more control over that. With it, we can map the short form (if that was provided by the test owner) of the unit to its long form.
So Firefox engineers aren't confused when reading the graphs. This mapping can be done at frontend level.
There's indeed a friction between us & test owners, but that can be addressed in the same PR which adds a new test.

I don't have any problems with eliminating this predefined list.
But the question is: are we okay with fixing situations like these after the fact (after Firefox engineers can potentially complain about not understanding what a unit means) or should we try and forestall them?
If we go with option #1, we could simply enforce these units to have a length of minimum 1.
If we got with option #2, either we stick to this list or require test owners to provide more than just "unit": "ms". We should require them to provide the short form and the long form.

Dave, what's your call?

Flags: needinfo?(igoldan) → needinfo?(dave.hunt)

Ah I see, thanks for the explanation. +1 for option 2 using the short and long form.

(In reply to Ionuț Goldan [:igoldan], Performance Sheriff from comment #4)

Dave, what's your call?

I want to make sure I understand what we're proposing. The existing units that concern me are 1/FPS, ms/frame, and frame interval. These all sound like similar units, but ultimately are likely all measured in ms. Could you provide some clarification? By long form, do you mean a description of the data? Do you have an example for demonstration purposes? I can imagine this being a valuable addition, and maybe we could make this required if the unit does not match against our list?

Flags: needinfo?(dave.hunt) → needinfo?(igoldan)

(In reply to Dave Hunt [:davehunt] [he/him] ⌚BST from comment #6)

I want to make sure I understand what we're proposing. The existing units that concern me are 1/FPS, ms/frame, and frame interval. These all sound like similar units, but ultimately are likely all measured in ms. Could you provide some clarification? By long form, do you mean a description of the data? Do you have an example for demonstration purposes?

By long form, I don't mean a description of the data.
Let me share some examples:

  • let's say we have ms. This is the short form for milliseconds
  • for FPS, the long form is frames per second
  • for KB, the long form is kilobytes
  • for %, the long form is percentage
  • and so on.

I can imagine this being a valuable addition,

Indeed.

and maybe we could make this required if the unit does not match against our list?

This sounds like a mix of solutions. I'm not a fan of complexity. I would rather require test owners to mandatory provide a short form + its long form + an optional description & avoid using the predefined list. Other than this, test owners have full flexibility & freedom of choice for a particular measurement unit.

Flags: needinfo?(igoldan)

After discussing with Dave a bit about this ticket, we'll actually go with option #1 for Q4/2019. We're more interested in simply surfacing the measurement units in a basic form.
If we need more features on this, we'll consider it in the next quarters.

Greg, thanks a lot for pointing this out!

Pushed by igoldan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c3a12ed20ab
Provide json schema support for measurement units r=octavian_negru,perftest-reviewers,sparky
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Blocks: 1584946
You need to log in before you can comment on or make changes to this bug.