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

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
12 days ago
11 days ago

People

(Reporter: Dexter, Assigned: mdroettboom)

Tracking

Details

Attachments

(5 attachments)

Reporter

Description

12 days ago

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.

Reporter

Updated

12 days ago
Assignee: nobody → alessio.placitelli
Priority: -- → P1
Reporter

Updated

12 days ago
Blocks: 1554860
Reporter

Comment 1

12 days ago
Posted file 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

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

Reporter

Comment 3

12 days ago

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.

Assignee: alessio.placitelli → 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?

Reporter

Comment 6

11 days ago

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

Reporter

Comment 8

11 days ago

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

I guess I slightly lean toward fixing the inconsistency.

Likewise :)

Status: NEW → RESOLVED
Closed: 11 days ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.