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

Can the exemplar filter and reservoir be merged? #3812

Closed
Tracked by #3756
MrAlias opened this issue Jan 10, 2024 · 3 comments · Fixed by #3820
Closed
Tracked by #3756

Can the exemplar filter and reservoir be merged? #3812

MrAlias opened this issue Jan 10, 2024 · 3 comments · Fixed by #3820
Assignees
Labels
[label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:metrics Related to the specification/metrics directory

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Jan 10, 2024

Exemplar filters filter measurements prior to offering them to an exemplar reservoir. This "pre-filtering" functionality overlaps with the exemplar reservoir which will also sample measurements it is offered. Do we need these two different objects or can they be simplified into one?

Scope

Single global On/Off switch

The exemplar filter has be stated to be an object associated with the entire SDK, not just a single metric pipeline like the exemplar reservoir. This means that it can be used to "globally" apply pre-filtering functionality (i.e. turn on or off exemplars). For this reason it is argued that exemplar filters should remain independent.

Conversely, exemplar filters cannot be applied more specific then the entire SDK. If a user want some instruments to offer all measurements, some to only offer sampled, and others to drop all they cannot achieve this using exemplar filters. For this reason it is argued that exemplar filters should be combinable with exemplar reservoirs directly.

Preventing repetition

Because exemplar filters are applied to the entire SDK, this means every exemplar reservoirs do not need to reproduce the functionality of the exemplar filters. If an exemplar filter was merged into the exemplar reservoir directly, the exemplar reservoir would need to be configurable with each filter setting or a separate reservoir would be needed for each combination of filter-to-reservoir (i.e one for keep all into a random naive reservoir, keep only sampled into a naive reservoir).

For this reason it is argued that exemplar filters should remain independent.

Proposals

Providing decorator exemplar reservoirs

  • The "AlwaysOn" exemplar filter is dropped
  • The "AlwaysOff' exemplar filter is replaced by a "drop" exemplar reservoir
  • The "TraceBased" exemplar filter is replaced with a "decorator" exemplar reservoir
    • This new reservoir would accept an exemplar reservoir on creation
    • All measurements it is offered are first checked to be sample prior to passing them to the decorated exemplar reservoir
  • The default exemplar reservoirs would all be changed to use the trace-based decorator exemplar reservoir
  • A "default" exemplar reservoir object would be exposed to users so they can still set global views for all instruments to that will appropriately decorate the default (instrument selected) exemplar reservoir

cc @jack-berg @jsuereth @jmacd

@MrAlias MrAlias added the spec:metrics Related to the specification/metrics directory label Jan 10, 2024
@jsuereth
Copy link
Contributor

The exemplar reservoir does not have access to the trace information associated with a measurement. Therefore, the trace based exemplar filter cannot be reproduced with an exemplar reservoir.

This should actually be false, according to the specification. ExemplarReservior SHOULD have access to context, especially since we need TraceContext for IDs we record.

I think the point @jmacd made was that you can highly optimise what information you unpack look at using a global filter by preventing a lot of allocations/boilerplate filtering before passing to the reservoir.

Proposal: Update the exemplar reservoir offer method to accept trace information

Again, this is already true in the specification.

Proposal: Providing decorator exemplar reservoirs

I understand the notion of a decorator. Can you expand how this would interact with configuration?

If you look at #3820 - I do a few things to help with this question:

  1. ExemplarFilter is merely an SDK-wide configuration parameter. The interface we define for it COULD be removed from the specification, as the values and their behavior is defined, and users are not able to provide custom implementations.
  2. I updated the compliance matrix to be inline with the status of the verbage of the specification.

Basically, my opinion, is that the specification today would require an SDK-wide filter configuration parameter of AlwaysOn, AlwaysOff or TraceBaseed. SDKs should be free to implement this using the decorator pattern if they desire. I think we could basically remove the interface definition of ExemplarFilter, and we won't lose anything important in the current Exemplar specificaiton. WDYT?

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 12, 2024

The exemplar reservoir does not have access to the trace information associated with a measurement. Therefore, the trace based exemplar filter cannot be reproduced with an exemplar reservoir.

This should actually be false, according to the specification. ExemplarReservior SHOULD have access to context, especially since we need TraceContext for IDs we record.

I think the point @jmacd made was that you can highly optimise what information you unpack look at using a global filter by preventing a lot of allocations/boilerplate filtering before passing to the reservoir.

Proposal: Update the exemplar reservoir offer method to accept trace information

Again, this is already true in the specification.

You are correct 👍 . I'll update the description.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jan 12, 2024

If you look at #3820 - I do a few things to help with this question:

  1. ExemplarFilter is merely an SDK-wide configuration parameter. The interface we define for it COULD be removed from the specification, as the values and their behavior is defined, and users are not able to provide custom implementations.
  2. I updated the compliance matrix to be inline with the status of the verbage of the specification.

Basically, my opinion, is that the specification today would require an SDK-wide filter configuration parameter of AlwaysOn, AlwaysOff or TraceBaseed. SDKs should be free to implement this using the decorator pattern if they desire. I think we could basically remove the interface definition of ExemplarFilter, and we won't lose anything important in the current Exemplar specificaiton. WDYT?

This sounds good to me 👍. I'm in favor of that change as a way to resolve this issue.

jsuereth added a commit to jsuereth/opentelemetry-specification that referenced this issue Jan 17, 2024
@reyang reyang added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Jan 24, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
…emetry#3820)

Fixes open-telemetry#2922
Fixes open-telemetry#3812 
Related to open-telemetry#3756

## Changes

- Cleans up language around exposing `ExemplarReservoir`. (Remove TODO,
e.g.)
- Remove `ExemplarFilter` interface but keep configuration options. (see
open-telemetry#3812)
- Clarify / simplify Spec Compliance matrix for existing state of the
Exemplar Specification.


Prototype in java:
open-telemetry/opentelemetry-java#5960
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants