MrAlias / otel-schema

Playground to prototype and investigate configuration schema proposals for OpenTelemetry
Apache License 2.0
2 stars 7 forks source link

add example configuration #6

Closed codeboten closed 1 year ago

codeboten commented 1 year ago

The propose configuration is a combination of the proposal in the description of #5 and of the kitchen-sink yaml. I've pulled limits, exporters, and resources out of the sdk block. I kept the rest of the configuration mostly intact.

Fix #5

tsloughter commented 1 year ago

Rethinking if I like the name references or not.

Why does the collector use this instead of yaml anchors? Is it to allow the feature to be used when writing json for configuration?

Feels like unnecessary work on the side of each parser if yaml and/or cue is used and there is built in support to reuse blocks of config.

codeboten commented 1 year ago

Some additional prior art to consider from the OpenTelemetry Operator:

kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
  name: my-instrumentation
spec:
  exporter:
    endpoint: http://otel-collector:4317
  propagators:
    - tracecontext
    - baggage
    - b3
  sampler:
    type: parentbased_traceidratio
    argument: "0.25"
EOF
tsloughter commented 1 year ago

Related to my comment about removing *_provider the Operator example shows setting a single exporter. It could be that there is a top level exporter configuration value which is used for traces, metrics and logs unless a per-Signal exporter definition is given. Then no duplication is needed, nor is a name or anchor.

tsloughter commented 1 year ago

I guess that actually follows the environment variable pattern where there is OTEL_EXPORTER_OTLP_ENDPOINT and then OTEL_EXPORTER_OTLP_TRACES_ENDPOINT.

jack-berg commented 1 year ago

It could be that there is a top level exporter configuration value which is used for traces, metrics and logs unless a per-Signal exporter definition is given. Then no duplication is needed, nor is a name or anchor.

This limits the configuration scenarios that can be expressed. How do I say that I want to export to zipkin and otlp for traces, with zipkin every 30 second and otlp every 60 seconds?

IMO the configuration should fall out from the programatic configuration of the SDK. Deviating from this means being more likely to run into things that can be expressed programmatically but not in a file. Applied to this topic, it means supporting an array of span processors, where the built in simple and batch span processors accept a mandatory exporter as an argument.

tsloughter commented 1 year ago

@jack-berg is your issue that you'd have to duplicate the otlp configuration if its a case that you want to use the sae OTLP exporter config for traces, metrics and logs but also a zipkin exporter for traces?

jack-berg commented 1 year ago

Not sure whether or not you're in favor of the operator's config, but I'm trying to say that it's not a good one to model after since it doesn't reflect how SDKs are actually configured.

Whether a single top level exporter:

  exporter:
    endpoint: http://otel-collector:4317

Or signal specific exporters:

  tracer_exporter:
    endpoint: http://otel-collector:4317
  metric_exporter:
    endpoint: http://otel-collector:4317

Neither of these reflect the reality that SDKs aren't configured with exporters. SDKs are configured with processors, where some of the built in processors require an exporter as an argument. Means that you can configure an SDK with custom processors that don't export at all, or with multiple processors each with a different exporter attached. We need to preserve these options in a file based config.

tsloughter commented 1 year ago

IMO the configuration should fall out from the programatic configuration of the SDK

I don't necessarily disagree with this but do think we'll wind up being considered "hard to configure". Maybe that is just how it has to be. In Erlang for the config file I have it so a simple set of top level options are available, so you could for example still configure the interval of trace export and the protocol used, without knowing how this relates to a tracer providers array of span processors. But the ability to configure each processor in the array is preserved by taking precedence over any processor that would be generated based on top level configuration.

It is hairy trying to support both simple and detailed configuration, so I can see only choosing one and going with detailed so nothing is lost.

jack-berg commented 1 year ago

The idea of supporting simple and detailed is interesting. Suppose we started by being able to express relatively complicated config. We could always add additional simpler options which effectively are just syntactic sugar for the more complex representation they translate too.

An argument for targeting complex config is that the environment based config already has a lot of the simple options covered, so restricting the config to those simple options has less utility.

codeboten commented 1 year ago

The ability to configure a different exporter(s) per signal has to be part of the configuration file as it already is covered by environment variables here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#exporter-selection

tsloughter commented 1 year ago

SDKs aren't configured with exporters. SDKs are configured with processors

@jack-berg they sort of are though. The env var is OTEL_TRACES_EXPORTER, not OTEL_BSP_TRACES_EXPORTER.

But I think I can get behind the strategy that the simple options are covered by the environment variables and the config file essentially maps directly to the SDK.

jack-berg commented 1 year ago

We've adjusted our approach from trying to merge an idealized config all at once to working in smaller chunks like #13, #14, #17, #19, #20.

@codeboten can we close this PR?

@svrnm I think you bring up some interesting points. Can you open individual issues to discuss?

codeboten commented 1 year ago

@jack-berg yup!