Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a count argument to histogram record API #3369

Open
jack-berg opened this issue Apr 4, 2023 · 9 comments
Open

Add a count argument to histogram record API #3369

jack-berg opened this issue Apr 4, 2023 · 9 comments
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback

Comments

@jack-berg
Copy link
Member

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 jack-berg added the spec:metrics Related to the specification/metrics directory label Apr 4, 2023
@reyang reyang added the area:api Cross language API specification issue label Apr 4, 2023
@reyang
Copy link
Member

reyang commented Apr 4, 2023

Would this also apply to other instrument types?

@jack-berg
Copy link
Member Author

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.

@reyang
Copy link
Member

reyang commented Apr 4, 2023

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?

@jack-berg
Copy link
Member Author

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.

@pellared
Copy link
Member

pellared commented Apr 5, 2023

very inefficient

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

@jack-berg
Copy link
Member Author

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.

@jmacd
Copy link
Contributor

jmacd commented May 3, 2023

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.

@MrAlias
Copy link
Contributor

MrAlias commented May 3, 2023

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.

@jmacd
Copy link
Contributor

jmacd commented Jul 27, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback
Projects
None yet
Development

No branches or pull requests

6 participants