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

Add support for filtering attribute keys for streams via an exclude list #4188

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Aug 19, 2024

Changes

This change adds details around supporting the creation of a stream with a set of excluded attribute keys. This functionality is supported in the Go implementation via the use of filters. The proposal in this PR is to address an issue in the configuration working group: open-telemetry/opentelemetry-configuration#112

Please provide a brief description of the changes here.

For non-trivial changes, follow the change proposal process.

This change adds details around supporting the creation of a stream with
a set of excluded attribute keys. This is already done in the Go implementation
and the proposal in this PR is to address an issue in the configuration
working group: open-telemetry/opentelemetry-configuration#112

Signed-off-by: Alex Boten <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 30, 2024
Signed-off-by: Alex Boten <[email protected]>
codeboten added a commit to codeboten/opentelemetry-configuration that referenced this pull request Sep 9, 2024
This follows open-telemetry/opentelemetry-specification#4188 to add support
to exclude attribute keys from stream configuration.

Signed-off-by: Alex Boten <[email protected]>
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM, but one note before merging:
I'll let maintainers comment if there is a need to mark this section as experimental, and wait for 2-3 implementations before marking as stable. This PR right now makes no explicit call out, so this is by default treated as stable spec, though just 1 language has it implemented.

@jack-berg
Copy link
Member

I'll let maintainers comment if there is a need to mark this section as experimental, and wait for 2-3 implementations before marking as stable. This PR right now makes no explicit call out, so this is by default treated as stable spec, though just 1 language has it implemented.

Its already possible to do this in the stable java implementation. I think the same is true for go. I think this is safe / benign enough to merge as stable without an experimental period.

@github-actions github-actions bot removed the Stale label Sep 14, 2024
@codeboten
Copy link
Contributor Author

Don't think the failure is related to this pr:

  ERROR: 1 dead links found in ./specification/compatibility/prometheus_and_openmetrics.md !
  [✖] https:/prometheus/prometheus/commit/d9d51c565c622cdc7d626d3e7569652bc28abe15#diff-bdaf80ebc5fa26365f45db53435b960ce623ea6f86747fb8870ad1abc355f64fR76-R83 → Status: 406

@trask
Copy link
Member

trask commented Sep 16, 2024

ERROR: 1 dead links found in ./specification/compatibility/prometheus_and_openmetrics.md !
[✖] prometheus/prometheus@d9d51c5#diff-bdaf80ebc5fa26365f45db53435b960ce623ea6f86747fb8870ad1abc355f64fR76-R83 → Status: 406

(re-run resolved for now)

@jack-berg
Copy link
Member

@cijothomas since you approved this and you're involved in dotnet / rust, is it safe to say that you don't see this as a problem for dotnet / rust?

@open-telemetry/javascript-maintainers / @open-telemetry/python-maintainers can you comment on whether or not it would be problematic to extend view stream configuration as described in this PR?

@cijothomas
Copy link
Member

@cijothomas since you approved this and you're involved in dotnet / rust, is it safe to say that you don't see this as a problem for dotnet / rust?

Correct.

Copy link
Member

@JamieDanielson JamieDanielson left a comment

Choose a reason for hiding this comment

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

This seems reasonable. In JS we already have an allow-list, so it would just need to be extended for a deny-list.

@jack-berg jack-berg merged commit ed85e12 into open-telemetry:main Sep 18, 2024
6 checks passed
@pichlermarc
Copy link
Member

This seems reasonable. In JS we already have an allow-list, so it would just need to be extended for a deny-list.

It's already implemented on the next branch and will ship with SDK 2.0 🙂

@codeboten codeboten deleted the codeboten/support-exclude-list-attribute-keys branch September 18, 2024 16:11
codeboten added a commit to open-telemetry/opentelemetry-configuration that referenced this pull request Sep 19, 2024
This follows open-telemetry/opentelemetry-specification#4188 to add support
to exclude attribute keys from stream configuration.

---------

Signed-off-by: Alex Boten <[email protected]>
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.