1468809 - Do not report expired keyed histograms (it breaks GeckoView too!)

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Eugen Sawin [:esawin]]

Attachment: gv_measurements-1.json

The GeckoView example app is crashing on startup on a fresh install with

F MOZ_Assert: Assertion failure: false (The number of restored buckets does not match with the on in the definition), at /ho
me/esawin/dev/g/toolkit/components/telemetry/TelemetryHistogram.cpp:3146

Comment 1

[Eugen Sawin [:esawin]]

Attachment: gv_measurements.json

Comment 2

[Alessio Placitelli [:Dexter]]

Thanks, I'll look into this.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1468809 - Do not snapshot expired keyed histograms.

This patch changes the snapshotting code for keyed histograms
so that requesting a snapshot does not report expired histograms.

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

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985661 [details]
Bug 1468809 - Do not snapshot expired keyed histograms.

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

Comment 6

[Chris H-C :chutten]

Comment on attachment 8985661 [details]
Bug 1468809 - Do not snapshot expired keyed histograms.

https://reviewboard.mozilla.org/r/251196/#review257524

::: toolkit/components/telemetry/TelemetryHistogram.cpp:397
(Diff revision 2)
>    const HistogramInfo& info = gHistogramInfos[histogramId];
> -  kh = new KeyedHistogram(histogramId, info);
> +  const bool isExpired = IsExpiredVersion(info.expiration());
> +
> +  // If the keyed histogram is expired, set its storage to the expired
> +  // keyed histogram.
> +  if (isExpired) {

This control flow is odd. I think it would be better either as if (isExpired && !gExpiredKeyedHistogram) or by adding the kh logic into this if/else block like

if (isExpired) {
  if (!gExpiredKeyedHistogram) {
  }
  kh = gExpiredKeyedHistogram;
} else {
  kh = new KeyedHistogram(...);
}

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8985661 [details]
Bug 1468809 - Do not snapshot expired keyed histograms.

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

Comment 8

[Chris H-C :chutten]

Comment on attachment 8985661 [details]
Bug 1468809 - Do not snapshot expired keyed histograms.

https://reviewboard.mozilla.org/r/251196/#review257532

Comment 9

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8985661 [details]
Bug 1468809 - Do not snapshot expired keyed histograms.

https://reviewboard.mozilla.org/r/251196/#review257542

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/469b11ffea7a
Do not snapshot expired keyed histograms. r=chutten,janerik

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/469b11ffea7a