1954857 - .accumulateSingleSample on a labeled timing distribution metric doesn't mirror the data through GIFFT

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Florian Quèze [:florian]]

My patches in bug 1953106 cause a test failure in toolkit/components/extensions/test/xpcshell/test_ext_eventpage_idle.js.

This change makes the test pass:

diff --git a/toolkit/components/glean/api/src/private/labeled_timing_distribution.rs b/toolkit/components/glean/api/src/private/labeled_timing_distribution.rs
--- a/toolkit/components/glean/api/src/private/labeled_timing_distribution.rs
+++ b/toolkit/components/glean/api/src/private/labeled_timing_distribution.rs
@@ -234,6 +234,25 @@ impl TimingDistribution for LabeledTimin
     }
 
     pub fn accumulate_single_sample(&self, sample: i64) {
+        extern "C" {
+            fn GIFFT_LabeledTimingDistributionAccumulateRawMillis(
+                metric_id: u32,
+                label: &nsACString,
+                sample_ms: u32,
+            );
+        }
+        let id = &self
+            .id
+            .metric_id()
+            .expect("Cannot perform GIFFT calls without a MetricId");
+        // SAFETY: We're only loaning to C++ data we don't later use.
+        unsafe {
+            GIFFT_LabeledTimingDistributionAccumulateRawMillis(
+                id.0,
+                &nsCString::from(&self.label),
+                sample.try_into().unwrap(),
+            );
+        }
         #[cfg(feature = "with_gecko")]
         if gecko_profiler::can_accept_markers() {
             gecko_profiler::add_marker(

Comment 1

[Chris H-C :chutten]

From bug 1941760 comment #4:

So we're clear to have GIFFT convert samples to time_unit units on mirror for timing_distribution metrics, but converting to millis for labeled_timing_distribution metrics if we want to go that route.

Not entirely certain why I didn't include this in the patch.

Comment 2

[Chris H-C :chutten]

Attachment: Bug 1954857 - Mirror calls to labeled_timing_distribution's sample APIs r?TravisLong

It's not consistent with the non-labeled case which converts to time_unit units
because labeled_timing_distribution doesn't know them.

But it's no more inconsistent than before, and unblocks further FOG Migration.

Hopefully in bug 1943453 we'll figure out a way to get around the weirdness.
And all else fails, Legacy Telemetry is set to be removed (along with GIFFT)
in the medium term.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3430f953afed
Mirror calls to labeled_timing_distribution's sample APIs r=TravisLong

Comment 4

[Norisz Fay [:noriszfay]]

Backed out for causing BR bustages

Backout link

Push with failures

Failure log

Comment 5

[Chris H-C :chutten]

Ack, need some #[cfg(feature = "with_gecko")] in here.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10296d061874
Mirror calls to labeled_timing_distribution's sample APIs r=TravisLong

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/10296d061874