Move the scalar ID checks to the public API

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

(Blocks 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

Blocks: 1275517
Priority: -- → P1
Whiteboard: [measurement:client]
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
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.
Attachment #8839868 - Flags: review?(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.
Attachment #8839868 - Flags: review?(gfritzsche) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/578d891011c3
Move the scalar ID checks to the public API. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/578d891011c3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.