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

Expose ExemplarReservoir as configuration option of View. #3820

Merged
merged 13 commits into from Jan 31, 2024

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Jan 12, 2024

Fixes #2922
Fixes #3812
Related #3756

Changes

  • Cleans up language around exposing ExemplarReservoir. (Remove TODO, e.g.)
  • Remove ExemplarFilter interface but keep configuration options. (see Can the exemplar filter and reservoir be merged? #3812)
  • Clarify / simplify Spec Compliance matrix for existing state of the Exemplar Specification.

Prototype in java: open-telemetry/opentelemetry-java#5960

specification/metrics/sdk.md Outdated Show resolved Hide resolved
spec-compliance-matrix.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Outdated Show resolved Hide resolved
specification/metrics/sdk.md Show resolved Hide resolved
@cijothomas
Copy link
Member

How is #2922 resolved by this PR? I am not able to see how that issue is being addressed here.

@MrAlias
Copy link
Contributor

MrAlias commented Jan 18, 2024

How is #2922 resolved by this PR? I am not able to see how that issue is being addressed here.

A user will be able to implement their own exemplar reservoir that accomplishes the behavior desired in #2922. With this change the metric pipeline will be able to accept and use that reservoir.

@cijothomas
Copy link
Member

Remove ExemplarFilter interface but keep configuration options.

Can you add this to changelog as well ? Also, its not clear from PR description why we are removing the ability to author custom ExemplarFilter? Is that temporarily done to unblock stable release? Or we don't see the need of custom ExemplarFilters at all?

@cijothomas
Copy link
Member

How is #2922 resolved by this PR? I am not able to see how that issue is being addressed here.

A user will be able to implement their own exemplar reservoir that accomplishes the behavior desired in #2922. With this change the metric pipeline will be able to accept and use that reservoir.

I see. It'd good to describe in PR desc, how is this achieved so future readers can know how to achieve. Otherwise, merging this PR would just close the associated issue, without a clear description of how it was closed.

@jsuereth
Copy link
Contributor Author

Remove ExemplarFilter interface but keep configuration options.

Can you add this to changelog as well ? Also, its not clear from PR description why we are removing the ability to author custom ExemplarFilter? Is that temporarily done to unblock stable release? Or we don't see the need of custom ExemplarFilters at all?

Yeah, there was concern in the Specification meeting that ExemplarFilter duplicates work w/ ExemplarReservoir and users may be confused when to use one vs. the other. Rationale behind the decision is in: #3812.

We're err-ing on "less in the initial release is better", and we have the ability to bring back the interface if needed in the future.

specification/metrics/sdk.md Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

@jsuereth I think we are more than ready to merge this one! Please resolve the conflicts in the compliance matrix, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can the exemplar filter and reservoir be merged? Attribute to indicate whether a Span is used as an Exemplar
8 participants