Add scope configuration properties

[jack-berg]

Corresponding to the following spec sections:

• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#tracerconfigurator
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#meterconfigurator
• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#loggerconfigurator

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.

[MrAlias]

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

The Go SIG does not plan to implement this FWIW.

[jack-berg]

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]

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:

• there is no deadlock between repositories (spec waits for implementation and usage, various repositories waits for stable spec)
• issues in implementation or usage can be found, and the spec adjusted if needed

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.

[marcalff]

The structure overall looks ok.

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

• node TracerConfigurator: make default_config required
• node TracerMatcherAndConfig: make name and config required
• node TracerConfig: make disabled required

[jack-berg]

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]

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]

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:

• A way for a user to opt in
• A way for implementations to opt out

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]

@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.

[marcalff]

Suggested change

	"type": ["object", "null"],
	"type": ["object"],

[marcalff]

Suggested change

	"type": ["string", "null"]
	"type": ["string"]

[marcalff]

Suggested change

	"type": ["object", "null"],
	"type": ["object"],

[marcalff]

Suggested change

	"type": ["string", "null"]
	"type": ["string"]

[marcalff]

The cleanup done for tracer needs to be applied to metrics and logs as well.

See suggested changes.

[marcalff]

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.

Yes, thanks for that.

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.

Relying on the default of the default just to have something less verbose affects clarity, but the pattern makes sense.

Agreed.

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.

Makes sense.
Agreed.