Closed Bug 1451288 Opened 2 years ago Closed Last year

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

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

Attachments

(1 file)

+++ 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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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?
Flags: needinfo?(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.
Flags: needinfo?(gfritzsche)
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.
Attachment #8964884 - Flags: review?(alessio.placitelli) → review+
See Also: → 1361960
Priority: -- → P1
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40ab295f68f7
Do not force histogram keys to be valid C++ identifiers; r=Dexter
https://hg.mozilla.org/mozilla-central/rev/40ab295f68f7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.