1340542 - Move the scalar ID checks to the public API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

As done in bug 1339365, we need to move the scalar id checks consistently to the public Scalars API.

This means we should move the calls at [1] to the public callable APIs, e.g. [2].

[1] - http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/toolkit/components/telemetry/TelemetryScalar.cpp#1011,1147
[2] - http://searchfox.org/mozilla-central/rev/ea31c4b64f34a29415a69fb768f8051495547315/toolkit/components/telemetry/TelemetryScalar.cpp#1336

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1340542 - Move the scalar ID checks to the public API.

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

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8839868 [details]
Bug 1340542 - Move the scalar ID checks to the public API.

https://reviewboard.mozilla.org/r/114474/#review117504

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:331
(Diff revision 1)
> +  // Make sure we don't get scalars from other tests.
> +  Unused << mTelemetry->ClearScalars();
> +
> +// Don't run this part in debug builds as that intentionally asserts.
> +#ifndef DEBUG
> +  Telemetry::ScalarSet(Telemetry::ScalarID::ScalarCount, static_cast<uint32_t>(1));

Don't use only `ScalarCount`, use some other `ScalarCount + highNumber` and `std::numeric_limits<uint32_t>::max()` as well.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:342
(Diff revision 1)
> +  Telemetry::ScalarSetMaximum(Telemetry::ScalarID::ScalarCount, NS_LITERAL_STRING("key1"), 1);
> +#endif

You still need to check that we didn't record anything into the scalars.

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8839868 [details]
Bug 1340542 - Move the scalar ID checks to the public API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114474/diff/1-2/

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8839868 [details]
Bug 1340542 - Move the scalar ID checks to the public API.

https://reviewboard.mozilla.org/r/114474/#review117806

r=me with the below fixed.

::: toolkit/components/telemetry/tests/gtest/TestScalars.cpp:331
(Diff revision 2)
> +  Telemetry::ScalarID scalarId = static_cast<Telemetry::ScalarID>(std::limits<uint32_t>::max());
> +  Telemetry::ScalarSet(scalarId, static_cast<uint32_t>(1));
> +  Telemetry::ScalarSet(scalarId, true);

As mentioned, use more than one id.
At least: `ScalarCount`, `ScalarCount + N`, `max()`.
Dito for the keyed ones below.

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8839868 [details]
Bug 1340542 - Move the scalar ID checks to the public API.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114474/diff/2-3/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/578d891011c3
Move the scalar ID checks to the public API. r=gfritzsche

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/578d891011c3