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

Switch Stream back to having an AttributeFilter field and add New*Filter functions #4444

Merged
merged 14 commits into from
Aug 25, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 14, 2023

The existing fix to #4156 (#4288) introduced a change that limited the functionality of the SDK. Specifically it prevents the solution of the type of problems found in #3765. This does the following to address #4156:

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #4444 (1fc005d) into main (f15ae16) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4444     +/-   ##
=======================================
- Coverage   78.9%   78.9%   -0.1%     
=======================================
  Files        254     255      +1     
  Lines      20642   20651      +9     
=======================================
+ Hits       16301   16308      +7     
- Misses      3996    3999      +3     
+ Partials     345     344      -1     
Files Changed Coverage Δ
attribute/set.go 77.7% <ø> (ø)
sdk/metric/instrument.go 93.3% <ø> (+1.2%) ⬆️
attribute/filter.go 100.0% <100.0%> (ø)
sdk/metric/pipeline.go 85.9% <100.0%> (-0.1%) ⬇️
sdk/metric/view.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@MrAlias MrAlias marked this pull request as ready for review August 14, 2023 21:58
@MrAlias MrAlias added this to the v1.17.0/v0.40.0 milestone Aug 14, 2023
@dashpole
Copy link
Contributor

Have we gotten this OK'd with a TC reviewer as being spec compliant?

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 15, 2023

Have we gotten this OK'd with a TC reviewer as being spec compliant?

Not yet. Still working on it.

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as long as it complies with the spec.

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is TC sign-off, then I Approve this.

sdk/metric/benchmark_test.go Outdated Show resolved Hide resolved
sdk/metric/meter_test.go Outdated Show resolved Hide resolved
sdk/metric/pipeline.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 15, 2023

It was pointed out by @jack-berg that having a filter function in the stream definition here will be similar to the Java approach: https://github.com/open-telemetry/opentelemetry-java/blob/bebf684436b0f54c0c81b19eba1dd9d12ff37532/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/ViewBuilder.java#L67-L77

There they allow a you to define a generic predicate to decide which attribute keys are retained.

However, that only allows attribute keys to be used as the predicate, not values.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 15, 2023

After a slack conversation with @jack-berg he pointed out that @reyang, @jsuereth, and @jmacd will have more context here. I have created open-telemetry/opentelemetry-specification#3664 and pinged them in the issue.

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this with the assumption that this is for beta release. The Spec compliance is still in process.

@MrAlias
Copy link
Contributor Author

MrAlias commented Aug 24, 2023

SIG meeting notes:

  • There seems to be TC approval of this
  • This is reverting to what is already released with the addition of the two new filter functions
    • Reverting for the release will align with where we hope to land
    • Reverting for the release will avoid any churn which would happen if we were to revert after the release
    • The addition of the two new filter functions, while an addition to the attribute API, seem useful even if the attribute filter field doesn't go through
  • The plan is to wait until tomorrow and then merge this and release if there are no objections between now and then

@MrAlias MrAlias merged commit 69611bd into open-telemetry:main Aug 25, 2023
21 checks passed
@MrAlias MrAlias deleted the attr-filter-revert branch August 25, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants