David Ashpole
David Ashpole
I changed it to check for time.IsZero()
Java doesn't appear to accept a timestamp (although it looks like it is internal): https://github.com/open-telemetry/opentelemetry-java/blob/0aacc55d1e3f5cc6dbb4f8fa26bcb657b01a7bc9/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/exemplar/ExemplarReservoir.java#L95, but JS does: https://github.com/open-telemetry/opentelemetry-js/blob/f99e7d9c2c37376e752f746308affa74fc7c30d2/packages/sdk-metrics/src/exemplar/ExemplarReservoir.ts#L33.
@MrAlias I was re-reading the specification, and I think the intent is that Offer is only called when the exemplar is _definitely_ going to be recorded (i.e. after the filter)....
> Does that mean we would keep the existing Offer method and just change the FilteredReservoir in this PR? Yes.
Please let me know if this is too large to review or too large for a last-minute change before a release. I can try to make a version which just...
To summarize my findings in https://github.com/open-telemetry/opentelemetry-go/pull/5544. * We should take a close look at the exemplar reservoir performance when exemplars are disabled. It currently makes up a substantial (~50%) portion...
> We should take a close look at the exemplar reservoir performance when exemplars are disabled. It currently makes up a substantial (~50%) portion of the overhead for the no-attributes...
I also found that the benchmark did not change if I swapped out the OTel prometheus exporter with a manual reader (which is expected). I'm removing the prometheus exporter label.
https://github.com/open-telemetry/opentelemetry-go/pull/5545 is a ~45% performance improvement for the zero-attributes case, and a ~20% performance improvement for the single-attribute case.
Would you mind either updating the description or opening an issue with the problem you encountered? In which scenarios are we leaking spans, and how did you encounter the issue?