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.

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

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...

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.

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?

(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..

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.

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

I guess I slightly lean toward fixing the inconsistency.

Likewise :)