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

propagator name should not be a blank string #172

Open
codeboten opened this issue Feb 18, 2025 · 1 comment · May be fixed by #173
Open

propagator name should not be a blank string #172

codeboten opened this issue Feb 18, 2025 · 1 comment · May be fixed by #173

Comments

@codeboten
Copy link
Contributor

If a user sets the following configuration, I would expect a configuration error to be returned:

propagator:
  composite: [""]

This would ensure that implementations follow the same pattern. The configuration above is currently valid as defined by the schema https://github.com/open-telemetry/opentelemetry-configuration/blob/main/schema/propagator.json, I suggest we add a NonEmptyString type and use that as the type for propagator configuration.

@jack-berg
Copy link
Member

If a user sets the following configuration, I would expect a configuration error to be returned:

I think it should return an error today.

Its a question of when the error should be returned. Today, a blank / empty string is a valid entry in propagator.composite, but when create is called it doesn't find any propagator called with blank / empty name and produces an error.

By adding an additional JSON schema constraint around the length, we arguably shift the error generation to when parse is called.

I say "arguably" because there's no consistency in how JSON schema tooling works. Some, like the tooling we use in opentelemetry-java, is great for generating bindings from the schema, but doesn't attempt to enforce the validation. So we implement all the validation when interpreting the in-memory model in create. But a language with JSON schema tooling which better incorporates the validation keywords might be able to produce an error in parse.

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