opentelemetry-specification icon indicating copy to clipboard operation
opentelemetry-specification copied to clipboard

Add a count argument to histogram record API

Open jack-berg opened this issue 2 years ago • 9 comments

I want to be able to record the same measurement to a histogram multiple times in one call.

For example, suppose I need to record a measurement with value 5, and attributes {foo=bar} to a histogram. In java I can call:

myHistogram.record(5, Attributes.of(stringKey("foo"), "bar"));

Now suppose that I need to record that same measurement to the histogram 50 times. In java I would have to do something like:

for (int i = 0; i < 50; i++) {
  myHistogram.record(5, Attributes.of(stringKey("foo"), "bar"));
}

This is very inefficient compared to if there was an API to record the 50 measurements in a single call, which might look something like:

// Record that the measurement with value 5 and attribute {foo=bar} was seen 50 times
myHistogram.record(5, Attributes.of(stringKey("foo"), "bar"), 50);

There's the idea of bound instruments, which if it existed, would allow you to obtain an handle once and record to it without having to duplicate the effort of obtaining the handle. But this is still inefficient compared to recording this in a single call. Recording in a single call would allow an implementation to look up the bucket a single time, allow the sum to be incremented a single time, and the calls to compute min / max would be invoked a single time.

This is a useful thing to do when recording durations in batch processing. When processing a batch, you want to record the processing duration of each item in the batch to the histogram, and all the items have the same duration.

We should consider extending the Histogram record operation to accept an optional count parameter.

jack-berg avatar Apr 04 '23 21:04 jack-berg

Would this also apply to other instrument types?

reyang avatar Apr 04 '23 21:04 reyang

Would this also apply to other instrument types?

I don't think so.

Counter and UpDownCounter both aggregate to sums, so a caller can multiply the value by the count to achieve the same affect:

for (int i = 0; i < 50; i++) {
  counter.add(5, Attributes.of(stringKey("foo"), "bar"));
}
// is equivalent to
counter.add(50 * 5, Attributes.of(stringKey("foo"), "bar"));

If you wanted to aggregate a counter to a histogram, then the difference between 5 * 50 and 50 distinct measurements of 5 would be relevant. However, this would suggest that histogram would have been a more appropriate instrument in the first place.

Gauges take last value, so the number of measurements is irrelevant.

jack-berg avatar Apr 04 '23 21:04 jack-berg

Counter and UpDownCounter both aggregate to sums, so a caller can multiply the value by the count to achieve the same affect:

What about context and exemplars?

reyang avatar Apr 04 '23 21:04 reyang

What about context and exemplars?

Good point. Recording 50 * 5 would skew the value for exemplars. Would be good to add this parameter for other instruments as well then.

jack-berg avatar Apr 04 '23 22:04 jack-berg

very inefficient

Do we have some benchmarks that would support this proposal and would confirm that the issue is not exaggerated?

pellared avatar Apr 05 '23 14:04 pellared

I can write a benchmark, but its pretty easy to anticipate what it will yield: Whatever overhead there is to record a single measurement will be duplicated the number of times the measurement needs to be recorded. In contrast, recording in a single call should be equivalent in overhead to a single invocation for a proper implementation.

So the efficiency drop of sequential according vs a batch is proportional to the size of the batch. If the batch size is 2, then maybe its not a big deal, but if the batch size is 10k, or 100k, then it's an increasingly big deal.

FYI, I'm a bit reluctant to write the benchmark because doing so requires prototyping this feature. Would like it if there was a little more support before I do that work.

jack-berg avatar Apr 05 '23 16:04 jack-berg

I don't think this corner-case is worth adding a new option to the histogram update operation, mainly because optionality is expensive in the OTel-Go API. I would prefer to add a new histogram method.

jmacd avatar May 03 '23 15:05 jmacd

optionality is expensive in the OTel-Go API

@jmacd can you expand on this?

Passing this as an option shouldn't be more overhead then passing attributes. It was the reason we added options to measurement methods in the first place.

MrAlias avatar May 03 '23 16:05 MrAlias

Sorry for the delay.

shouldn't be more overhead then passing attributes

This issue was used to justify make attribute passing part of an optional pattern, so now passing attributes is expensive whereas it would not be without the optional pattern. I am referring to the expense of allocating one struct to implement the option interface and one slice for passing the options, which came about because what I see here as a feature request.

My operating philosophy is that instruments should serve a single purpose. From this point of view, if we wanted a histogram with a multi-record method, we should add a new instrument called MultiHistogram and let its Record() method take an extra argument. I would use similar reasoning to argue for a new instrument that is a Histogram of timing measurements.

If I were pressed to say how I want to see this problem solved, however, I would recommend we learn about metric-event multiplicity from the context. The user who is in a position to use this feature would set the context once and then all their synchronous metrics operations would be multiplied. E.g., a new context interface such as MetricsAdjustedCount(context) would be created and apply to all synchronous instrument events: for Counters it multiplies the Sum, for Histograms it multiples the Count, (and for synchronous Gauges it is a Noop).

jmacd avatar Jul 27 '23 21:07 jmacd