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

flag includes invalid characters #29158

Closed
codeboten opened this issue Nov 13, 2023 · 10 comments · Fixed by #29122
Closed

flag includes invalid characters #29158

codeboten opened this issue Nov 13, 2023 · 10 comments · Fixed by #29122
Labels
bug Something isn't working exporter/googlemanagedprometheus Google Managed Prometheus exporter release:blocker The issue must be resolved before cutting the next release

Comments

@codeboten
Copy link
Contributor

codeboten commented Nov 13, 2023

Component(s)

exporter/googlemanagedprometheus

What happened?

See https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/6852247654/job/18630332796?pr=29122

go test -race -timeout 300s -parallel 4 --tags="" -cover ./... -covermode=atomic -args -test.gocoverdir="/home/runner/work/opentelemetry-collector-contrib/opentelemetry-collector-contrib/coverage/unit"
?   	github.com/open-telemetry/opentelemetry-collector-contrib/exporter/googlemanagedprometheusexporter/internal/metadata	[no test files]
panic: invalid ID "gcp.untyped_double_export": invalid character(s) in ID
@codeboten codeboten added the bug Something isn't working label Nov 13, 2023
@codeboten
Copy link
Contributor Author

@mx-psi

@github-actions github-actions bot added the exporter/googlemanagedprometheus Google Managed Prometheus exporter label Nov 13, 2023
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@mx-psi
Copy link
Member

mx-psi commented Nov 13, 2023

Flag was added on GoogleCloudPlatform/opentelemetry-operations-go/pull/668, so not recent. I didn't catch it while working on open-telemetry/opentelemetry-collector/pull/8766 because it was not defined on contrib itself but rather on an external repository, but I am still confused as to why contrib-tests passed 🤔

@damemi @dashpole what the status of this feature gate? For context, we were planning on disallowing _ on feature gates for now to have a conservative charset for 1.0. The simplest solution is to allow underscores, but would want to confirm first that this feature gate is not going away anytime soon.

@dashpole
Copy link
Contributor

The feature gate is still needed. It is for an internal (to google) use of the collector, so we don't advertise it in our README. It will be removed without graduation after open-telemetry/opentelemetry-specification#3058 is fixed, and implemented in the collector.

Is there a way to exempt that feature gate? Otherwise, I can update the GCP exporter to use a different name. What characters are allowed?

@mx-psi
Copy link
Member

mx-psi commented Nov 13, 2023

@dashpole ASCII alphanumerics and dots are the allowed characters https://github.com/open-telemetry/opentelemetry-collector/blob/23500dda16b565d5f15c535e5a9d22927e4700d2/featuregate/registry.go#L21-L22

Using camel case or upper camel case (i.e. gcp.untypedDoubleExport or gcp.UntypedDoubleExport) would be consistent with all other feature gates in contrib at least. Would it be possible to change it to one of the two?

@codeboten
Copy link
Contributor Author

@mx-psi @dashpole since this will be blocking the release, I would suggest allowing the underscores for now. WDYT?

@mx-psi
Copy link
Member

mx-psi commented Nov 13, 2023

Filed open-telemetry/opentelemetry-collector/pull/8865 in case we want to allow underscores instead of fixing this gate. If the fix by David can be filed by EOD, I think we should go with that, otherwise my PR should be mergeable as is.

@codeboten
Copy link
Contributor Author

Adding @jpkrohling to ensure there isnt a release made until this is cleared up. I also labeled the PR as release:blocker

@codeboten
Copy link
Contributor Author

Thanks for the quick fix @dashpole, I've updated the dependency here: 6b4398c

@dashpole
Copy link
Contributor

I just cut a release. Can you update to v0.45.0?

codeboten pushed a commit that referenced this issue Nov 14, 2023
Also updated the references to NewNoopTracerProvider.

Fixes #29158

---------

Signed-off-by: Alex Boten <[email protected]>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
Also updated the references to NewNoopTracerProvider.

Fixes open-telemetry#29158

---------

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
bug Something isn't working exporter/googlemanagedprometheus Google Managed Prometheus exporter release:blocker The issue must be resolved before cutting the next release
Projects
None yet
3 participants