1428888 - Allow C++ to accumulate multiple samples into a categorical histogram in one call

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Aditya Bharti [:adbugger]]

As per bug 1364043, the C++ Telemetry::Accumulate function was extended to support adding multiple samples to a histogram in one call. This bug is to extend the same support for categorical histograms. See comment #27 on bug 1364043 for more information.

Comment 1

[Aditya Bharti [:adbugger]]

I would like to work on this bug too. I have a patch in the works and will upload in a few minutes. In the meantime, I am assigning this bug to myself.

Comment 2

[Aditya Bharti [:adbugger]]

Attachment: Accept a histogram id and multiple nsCString labels

This version of the API accepts a histogram id and a nsTArray<nsCString> of labels to accumulate into categorical histograms.

Created another temporary array to hold uint32_t label ids in TelemetryHistogram::AccumulateCategorical function in TelemetryHistogram.cpp file. That along with the NS_FAILED check ensures that either all or none of the array labels are accumulated. I thought an erroneous label in the middle of the array would have caused a lot of problems otherwise.

Added an extra test case.

Has been built locally and tested locally with gtest.

Will remove TODOs in final patch.

Comment 3

[Chris H-C :chutten]

Comment on attachment 8945211 [details] [diff] [review]
Accept a histogram id and multiple nsCString labels

Review of attachment 8945211 [details] [diff] [review]:
-----------------------------------------------------------------

This is good so far!

I'm not sure how many C++ consumers use the string API. From searching, it seems that the enum case is far more commonly used. Can we expect that implementation in a subsequent patch on this bug?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +2027,5 @@
> +  }
> +
> +  StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +
> +  nsTArray<uint32_t> intSamples;

Since we know the size this will have to be, you can create this with the necessary capacity to save it from having to resize itself.

@@ +2028,5 @@
> +
> +  StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> +
> +  nsTArray<uint32_t> intSamples;
> +  for(nsCString label: aLabels){

copy elision would probably save us here, but might be better to make it clear we want nsCString&.

Also, there should be a space between for and (

@@ +2036,5 @@
> +    }
> +    intSamples.AppendElement(labelId);
> +  }
> +
> +  for(uint32_t sample: intSamples){

It pains me to see two loops for a single operation, but your rationale of accumulating all or none is excellent. Please put a comment somewhere in this function explaining that rationale.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +565,5 @@
> +  // Check that the value stored in the histogram matches with |kExpectedValue|
> +  uint32_t uValue = 0;
> +  JS::ToUint32(cx.GetJSContext(), value, &uValue);
> +  ASSERT_EQ(uValue, kExpectedValue) << "The histogram is not returning expected value";
> +}

Need a test for your "all or nothing" behaviour. You can roll it into this case or give it its own case, but accumulating at least one valid and one invalid label to ensure that the valid label goes uncounted is a necessary case to check.

Comment 4

[Aditya Bharti [:adbugger]]

(In reply to Chris H-C :chutten from comment #3)

> I'm not sure how many C++ consumers use the string API. From searching, it
> seems that the enum case is far more commonly used. Can we expect that
> implementation in a subsequent patch on this bug?

Yup, I'll handle the enum case in a subsequent patch.

I am making the necessary changes. Once the bugs in this string API are ironed out, I'll get back on the enum case.

Comment 5

[Aditya Bharti [:adbugger]]

Attachment: Test for no accumulation when a bad label is present in array

Made the required changes.

Tested for the no accumulation condition in the same test case since I was accumulating data anyway.

Added a comment about the two loops and moved the lock acquiring statement further down, just before the internal_Accumulate loop in TelemetryHistograms.cpp. Seemed sensible to acquire the lock as close to the critical section as possible.

If there are no other issues, I'll work on the enumValues case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be9b71ee3f98545645c9825605cd26f5f083c322

Comment 6

[Chris H-C :chutten]

Comment on attachment 8945601 [details] [diff] [review]
Test for no accumulation when a bad label is present in array

Review of attachment 8945601 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch about moving the lock. I had to double-check that we didn't need it to ensure someone wasn't writing to gHistogramInfos while we were reading from it, but it's read-only.

Comment 7

[Aditya Bharti [:adbugger]]

Attachment: Telemetry::Accumulate() now supports both (histogram id, string labels array) and (enumValues array)

Figured out what the problem was. C++ requires template functions to be implemented in the header file itself (.h) or in a separate template specification (.tpp) file. I was putting the function body in Telemetry.cpp and that was the reason for the the undefined reference.

This patch supports both the extensions to the Telemetry::Accumulate() API, string labels and enumValues. 

For the string labels case, there is a test case to ensure that we do not accumulate anything in the case of an invalid label being present in the array.

For the enumValues array, the class template itself ensures that we do not have any enum mismatch within the array, (I think that is how it works, please correct me if I'm wrong), so I haven't added any specific functionality or test case to handle that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c87621341e6f83d675fe65793f0797c2f78579db

Comment 8

[Aditya Bharti [:adbugger]]

Also, should I create another bug for keyed categorical histograms, or do that in a subsequent patch on this bug itself?

Comment 9

[Chris H-C :chutten]

(In reply to Aditya Bharti [:adbugger] from comment #8)
> Also, should I create another bug for keyed categorical histograms, or do
> that in a subsequent patch on this bug itself?

I'd lean towards filing another bug to cover it at this stage.

While you're filing that bug, could you also file one to update the documentation? We should start advertising that these APIs exist so that callers can update their code.

Comment 10

[Chris H-C :chutten]

Comment on attachment 8946114 [details] [diff] [review]
Telemetry::Accumulate() now supports both (histogram id, string labels array) and (enumValues array)

Review of attachment 8946114 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

You are right about the type-safety of the enum types. That was one of the benefits of implementing the labels this way, though I don't know if we ever considered this particular use case during their design :)

Comment 11

[Aditya Bharti [:adbugger]]

Marking for checkin

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59aa68657042
Allow C++ to accumulate multiple samples into a categorical histogram in one call r=chutten

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/59aa68657042