1566764 - Consider allowing more characters (e.g. `-`) in the labels for labelled metrics

Status: RESOLVED FIXED

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

Description

[Alessio Placitelli [:Dexter]]

Thanks to Chris' investigation (bug 1552507), we noticed that Fenix is currently not reporting full search data. See the specific investigation here by boek.

This is due to the fact that Fenix is using the engine name as the label for search_count.

However search engine name, in non en-US locales, might contain characters other than the ones we allow for labels. A list of default search engines is available here. Even if we were to use the engine code instead of the name, as we do on desktop, we'd still fail to validate due to the extensive use of - in the engine code name.

This is our current regex for validating labels.

Comment 1

[Alessio Placitelli [:Dexter]]

Thoughts on the above?

I think that, at the very least, we should find a way to allow '-' in label names, to keep feature parity with desktop. This might be important for analysis across desktop and mobile. The Fenix might need to switch to use 'code' instead of the engine 'name', to have a consistent behaviour with respect to Desktop.

Comment 2

[Jan-Erik Rediger [:janerik]]

(uh, I typed this comment before, but it wasn't saved?)

- sounds ok to me, should be easy to handle in code. What's the pipeline's opinion on that?
What was the initial decision for such a restricted set of allowed characters (the fact that it gets transformed into a table column name?)?

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #2)

(uh, I typed this comment before, but it wasn't saved?)

- sounds ok to me, should be easy to handle in code. What's the pipeline's opinion on that?
What was the initial decision for such a restricted set of allowed characters (the fact that it gets transformed into a table column name?)?

Frank, would having a dash - in Glean SDK labels be a problem for the pipeline?

Comment 4

[Travis Long [:travis_]]

Barring any complaints from the pipeline side of things with the -, I don't have any problems with adding dashes to the allowed separators.

Comment 5

[Frank Bertsch [:frank]]

Labels do not transformed to columns, they are simply strings - this should be good to go for the pipeline.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: Allow '-' in the schema

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: Change the glean_parsers

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: Changes to glean-ac

Comment 9

[Alessio Placitelli [:Dexter]]

I will change glean-core as part of this bug as well.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: Changes to glean-core

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: Bump the parsers's version in glean-ac

The ones for glean-core are already bumped as part of the related PR.