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 the internal experimental metric feature package #4715

Merged
merged 11 commits into from
Dec 6, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Nov 13, 2023

Used to enable specification defined features. Such as:

This is the first step in adding these features.

Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Merging #4715 (ec66f05) into main (9ccb0c6) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4715   +/-   ##
=====================================
  Coverage   81.9%   82.0%           
=====================================
  Files        224     225    +1     
  Lines      18208   18239   +31     
=====================================
+ Hits       14930   14961   +31     
  Misses      2989    2989           
  Partials     289     289           
Files Coverage Δ
sdk/metric/internal/x/x.go 100.0% <100.0%> (ø)

@dashpole
Copy link
Contributor

Did you consider modeling this after feature-gates in the collector? https://github.com/open-telemetry/opentelemetry-collector/tree/main/featuregate. If we used it directly, it would mean collector users could configure OTel Go's feature gates using the collector's feature-gates flag, which would be neat. We would have to wait for the v1 release to avoid a circular unstable dependency.

Assuming we don't want to depend on it directly, could we follow the same pattern, and use a OTEL_GO_FEATURE_GATES env var with k0=v0,k1=v1 format, and defined stages? Or is that overkill?

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 14, 2023

Did you consider modeling this after feature-gates in the collector? https://github.com/open-telemetry/opentelemetry-collector/tree/main/featuregate. If we used it directly, it would mean collector users could configure OTel Go's feature gates using the collector's feature-gates flag, which would be neat. We would have to wait for the v1 release to avoid a circular unstable dependency.

Assuming we don't want to depend on it directly, could we follow the same pattern, and use a OTEL_GO_FEATURE_GATES env var with k0=v0,k1=v1 format, and defined stages? Or is that overkill?

I haven't looked at this yet. I'll be sure to take a look.

One question I have though, is how will uses distinguish between their SDK features and collector features? For example, given we will support different features than the collector but a user may define them in the same environment variable does that mean we need to drop unknown? Do we log this error?

@dashpole
Copy link
Contributor

One question I have though, is how will uses distinguish between their SDK features and collector features? For example, given we will support different features than the collector but a user may define them in the same environment variable does that mean we need to drop unknown? Do we log this error?

We would have to decide how to handle errors from parsing the flags, including when users set feature gates that don't exist. Writing to the global error handler seems reasonable.

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 14, 2023

@dashpole I don't see in that linked package where the environment variable OTEL_GO_FEATURE_GATES is supported. Is this an extension somewhere, or am I just bad at searching?

@MrAlias
Copy link
Contributor Author

MrAlias commented Nov 14, 2023

@dashpole does featuregate support processing configuration? That was an ask from here that I think is useful.

We would need to support additional environment variables if it does right? Or could we add it to that package?

@dashpole
Copy link
Contributor

It does not support configuration. We would need additional env vars or something else.

@dashpole
Copy link
Contributor

I don't see in that linked package where the environment variable OTEL_GO_FEATURE_GATES is supported. Is this an extension somewhere, or am I just bad at searching?

We would have to add that. It only supports flags today. Or, we could require users to register flags to use it, which has the advantage of making users decide how to handle parsing failures (so they don't need to use the global error handler)

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Can we add tests?

sdk/metric/internal/x/x.go Outdated Show resolved Hide resolved
sdk/metric/internal/x/x.go Show resolved Hide resolved
sdk/metric/internal/x/x_test.go Outdated Show resolved Hide resolved
sdk/metric/internal/x/x_test.go Show resolved Hide resolved
@MrAlias MrAlias merged commit d37d851 into open-telemetry:main Dec 6, 2023
25 checks passed
@MrAlias MrAlias deleted the metric-x branch December 6, 2023 18:36
@MrAlias MrAlias added this to the v1.22.0 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants