1556684 - Investigate why some `metrics` pings fail to validate due to labelled metrics

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

As found in bug 1556418 comment 4, some metrics pings fail to validate.

This is probably due to the length restriction for labels in labelled metrics.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: faling_ping.json

This ping fails to validate against this schema with the following error:

String 'search.default_engine.submission_url' exceeds maximum length of 30.
moz://mozilla.org/schemas/glean/ping/1#/properties/metrics/properties/labeled_counter/additionalProperties/propertyNames/maxLength

Comment 2

[Michael Droettboom [:mdroettboom]]

This is indeed exceeding the max length of 30 for labels. However, it remains a mystery (to me) how it got that far.

Here's where that limit is enforced (at build time) for static labels: https://github.com/mozilla/glean_parser/blob/c3a820965f0e258fb1381bd159a1f74e626c31e1/glean_parser/schemas/metrics.1-0-0.schema.yaml#L41

Here's where the limit is enforced (at run time) for dynamic labels: https://github.com/mozilla-mobile/android-components/blob/2b6ee4bc993f26431a7045482fd59d5a3a585889/components/service/glean/src/main/java/mozilla/components/service/glean/private/LabeledMetricType.kt#L107

And here's the enforcement on the pipeline (resulting in the message in the above comment): https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/866d46c128d201eda9cbc8a81946bff426928e2c/schemas/glean/metrics/metrics.1.schema.json#L175

All of these are at 30...

Comment 3

[Alessio Placitelli [:Dexter]]

From Jan-Erik, on IRC:

it complains about the "search.default_engine.submission_url" in the labeled_counter under the glean.error.invalid_value
that is the error reporting part
error reporting uses labeled counters, but doesn't go through the labeled counter storage implementation

As proposed, the solution seems to be allowing labels to be as long as metric ids can be, since we'll be using metric ids in errors.

Comment 4

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request

Comment 5

[Michael Droettboom [:mdroettboom]]

I filed a pull request to pipeline-schemas to increase the label length. This at least fixes the immediate bug of pings being rejected.

One loose end -- do we want to update glean to allow these longer labels for everyone, or just internally by the error reporting mechanism as it is now?

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Michael Droettboom [:mdroettboom] from comment #5)

One loose end -- do we want to update glean to allow these longer labels for everyone, or just internally by the error reporting mechanism as it is now?

This is a good question. Would having this distinction bite us back in glean-core? I'd prefer avoiding any special case, if possible. It would make debugging potential problems easier..

Comment 7

[Michael Droettboom [:mdroettboom]]

Yeah -- on the one hand, (1) I think keeping labels as short as possible is good. On the other hand, (2) I think this "deliberate inconsistency" of just updating the schema but not all the other parts is bound to cause confusion down the road.

I guess I slightly lean toward fixing the inconsistency.

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Michael Droettboom [:mdroettboom] from comment #7)

I guess I slightly lean toward fixing the inconsistency.

Likewise :)

Comment 9

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request

Comment 10

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request

Comment 11

[Michael Droettboom [:mdroettboom]]

Attachment: GitHub Pull Request

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad35b87f9e74
[components] 1556684: Update the label length limit from 30 -> 61 characters