1451288 - Do not force histogram keys to be valid C++ identifiers

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1423715 +++

While working on bug 1423715 we discovered that there is no need to constrain the histogram keys as we currently do so that they only use characters that would be valid in a C++ identifier.

Since we're not generating any C++ identifiers from those we can drop the constraint, and in fact we've already been recording keys that were incompatible with it.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1451288 - Do not force histogram keys to be valid C++ identifiers;

Review commit: https://reviewboard.mozilla.org/r/233618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233618/

Comment 2

[Alessio Placitelli [:Dexter]]

We had this limitation in place as we were planning to introduce histogram keys enum in bug 1361960. However, that still has to happen. Moreover, we are not enforcing the same limitations for keys at runtime. I would say let's consider dropping bug 1361960, stop enforcing valid c++ identifiers for keys at build time and re-evaluate the problem if we stumble upon a performance bottleneck in the future.

Georg, what do you think?

Comment 3

[Georg Fritzsche [:gfritzsche]]

(Sorry for the long delay)
It sounds like you have the impact considered Alessio, i leave this call to you.
If you need feedback, you can loop in someone else from our Telemetry client team.

Comment 4

[Alessio Placitelli [:Dexter]]

Comment on attachment 8964884 [details]
Bug 1451288 - Do not force histogram keys to be valid C++ identifiers;

https://reviewboard.mozilla.org/r/233618/#review242218

I think we can move forward with this. We don't even have to [update the docs](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#keys), as the enforced constraint wasn't even documented. I will close bug 1361960 as won't fix for now. We can revisit the decision when and if we decide we'll need the optimization.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40ab295f68f7
Do not force histogram keys to be valid C++ identifiers; r=Dexter

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/40ab295f68f7