codecov[bot]
commented
1 week ago Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 84.4%. Comparing base (
1cefb17) to head (93ec4fa).
Closed dashpole closed 1 day ago
codecov[bot]
commented
1 week ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 84.4%. Comparing base (
1cefb17) to head (93ec4fa).
MrAlias
commented
1 week ago The timestamp arg is something recommended by the specification: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplarreservoir
This may be necessary now, but when we accept a user defined reservoir this seems like an important parameter.
Would passing a zero value time from the SDK and check for this zero value in the reservoir to then call now be a better approach?
dashpole
commented
1 week ago I changed it to check for time.IsZero()
dashpole
commented
1 week ago 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.
dashpole
commented
1 week ago @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). We should probably have one Offer to the (non-public) FilteredReservoir which does not have a timestamp argument, and one Offer to the (to-be-public) Reservoir which does have the timestamp argument.
MrAlias
commented
1 week ago We should probably have one Offer to the (non-public) FilteredReservoir which does not have a timestamp argument, and one Offer to the (to-be-public) Reservoir which does have the timestamp argument.
I'm not sure I fully understand this implementation, but in general this sounds correct to me.
Does that mean we would keep the existing Offer method and just change the FilteredReservoir in this PR?
dashpole
commented
1 week ago Does that mean we would keep the existing Offer method and just change the FilteredReservoir in this PR?
Yes.
dashpole
commented
5 days ago 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 fixes the issue with as small of a diff as possible.
Part of addressing https://github.com/open-telemetry/opentelemetry-go/issues/5542.
Motivation
This removes the
time.Now()call from filtered-out Exemplars by only invokingtime.Now()after the filtering decision is made. This improvement is especially noticeable for measurements without any attributes.Changes
exemplar.Filter, which is a filter function based on the context. It will not be user-facing, so we can always add other parameters later if needed.exemplar.FilteredReservoir, which is similar to a reservoir, except it does not receive a timestamp. It gets the current time after the filter decision has been made. It uses generics to avoid the call to exemplar.NewValue(), since it is internal-only.exemplar.Reservoiris left as-is, so that it can be made public when exemplars are stable. It still includes a timestamp argument.exemplar.Dropis now anexemplar.FilteredReservoirinstead of aReservoir, since we don't need a Reservoir to store things in if the measurement is always dropped.