open-telemetry / opentelemetry-configuration

JSON Schema definitions for OpenTelemetry file configuration
Apache License 2.0
41 stars 17 forks source link

Add scope configuration properties #140

Open jack-berg opened 4 days ago

jack-berg commented 4 days ago

Corresponding to the following spec sections:

I believe this is the first time we've discussed adding properties which are experimental in the spec. And I want to use this as an opportunity to work out the details on how experimental properties work and their intersection with stability guarantees we still need to work out (#89).

The content in this PR needs review independent of the experimental conversation, and I'd like to find a way to separate the two issues.

One way we could proceed is to evaluate this PR ignoring the fact that the corresponding spec if experimental. Open an issue to find a resolution to experimental config properties, and indicate that the issue is blocking the next release (0.4.0). I would then tackle that issue as a followup to this, which I'm quite motivated to do because its a critical task on the path to stabilizing.

jack-berg commented 4 days ago

These are not stable parts of the specification. Do we want to add these?

See PR description. I think we need to find a path forward for having experimental portions of the data model, and figured this is a good problem domain to explore that.

The Go SIG does not plan to implement this FWIW.

I'm interested in hearing more about this.. do you mean Go doesn't plan to implement while the feature is experimental, or doesn't plan to implement ever? If Go doesn't plant to implement ever, is it because of some problem with the feature, issues with compatibility, or something else?

marcalff commented 4 days ago

These are not stable parts of the specification. Do we want to add these?

See PR description. I think we need to find a path forward for having experimental portions of the data model, and figured this is a good problem domain to explore that.

I agree.

If a spec is experimental, we should look at it sooner rather than later, so that:

The Go SIG does not plan to implement this FWIW.

I'm interested in hearing more about this.. do you mean Go doesn't plan to implement while the feature is experimental, or doesn't plan to implement ever? If Go doesn't plant to implement ever, is it because of some problem with the feature, issues with compatibility, or something else?

The C++ SIG is implementing this.

jack-berg commented 1 day ago

For a second pass, I think a lot of properties should be required, for example for traces:

I think this is a good opportunity to add schema modeling guidance for when a field should be required or not.

IMO, the general guidance should be something to the effect of: A field should be required if the spec explicitly states its required, or implies that its required by not indicating what should happen in its absence (i.e. no default is defined).

So .tracer_provider.processors[].batch.exporter is required, but .tracer_provider.processors[].batch.schedule_delay is not because the spec tells us what the default value is.

But this guidance alone isn't enough. For example, consider .tracer_provider.processors[].batch.exporter.otlp.headers[].name (and header value). The spec doesn't say anything about whether empty / null entries should generate an error or just be skipped. If we allow these entries to have semantics where entries with a null / empty name or value are skipped, then we could support things like the following, without erroring when the env var is not set. This is a nice feature for users:

tracer_provder:
  processors:
    - batch:
         exporter:
           otlp:
             headers:
               - name: ${EXTRA_HEADER_NAME_1}
                  value: ${EXTRA_HEADER_VALUE_1}

But as a counter argument, we already have the .tracer_provider.processors[].batch.exporter.otlp.headers_list property which helps satisfy this use case.

Anyway, it probably needs more analysis to extract general guidance.

Applied here:

node TracerConfigurator: make default_config required

The spec has defaults for each of the properties. Allowing default_config to be omitted allows users to count on the these defaults and selectively turn off scopes, resulting in less verbose config.

node TracerMatcherAndConfig: make name and config required

Agreed.

node TracerConfig: make disabled required

In a future where TracerConfig has more properties than just disabled, a user might want to rely on the default for disabled (i.e. false - scopes are enabled by default) and only set one of the other properties. It does feel a bit weird at the moment to have a type with a single property which is not required. I suppose we could come back and relax the requirement later if / when TracerConfig is extended.

MrAlias commented 1 day ago

I'm interested in hearing more about this.. do you mean Go doesn't plan to implement while the feature is experimental, or doesn't plan to implement ever? If Go doesn't plant to implement ever, is it because of some problem with the feature, issues with compatibility, or something else?

It's hard to say for certain what the future holds, but as of now the Go SIG does not plan to implement this ever. Defining singularly at the LoggerProvider if a scope is disable or enable is more restrictive than and conflicts with defining at the processor level. We plan to support optimization with our Logger.Enabled method utilizing processor aware state and as such plan to have this value configured there.

MrAlias commented 1 day ago

See PR description. I think we need to find a path forward for having experimental portions of the data model, and figured this is a good problem domain to explore that.

It seems like we are going to need two things for experimental configuration:

Users need to be able to validate their configuration with experimental features. However, implementations need to have a way they can signal this feature is not supported.

This seems adjacent to the instrumentation configuration in that it is something that would be an "add on" to the already stabilized configuration.

jack-berg commented 1 day ago

@MrAlias - thinking about this more.. My goal here was to use scope config as a way to tease out these mechanisms to model experimental stuff, allow users to opt in, etc.

But I realized that we're already modeling an experimental portion of the spec in the instrumentation config block. So I can use that config as the canary for answering these questions.

And then whatever we come up with can apply to scope config.

So I'd like to put this PR on hold and use .instrumentation instead.