open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
326 stars 157 forks source link

Configuration proposal #225

Closed codeboten closed 1 year ago

codeboten commented 1 year ago

This OTEP is the result of the Configuration working group. See https://github.com/open-telemetry/opentelemetry-specification/issues/2920 for additional details.

cc @MrAlias @jack-berg

On-going prototypes:

carlosalberto commented 1 year ago

Left a pair of comments but overall LGTM. One thing I'm curious about though, is how to individually enable/disable instrumentation libraries (which currently is exposed by both Java and DotNet auto instrumentation), e.g. OTEL_INSTRUMENTATION_QUARTZ_ENABLED=false

cc @pellared

pellared commented 1 year ago

@open-telemetry/dotnet-instrumentation-maintainers PTAL

dpk83 commented 1 year ago

I filed an issue for .NET specific configuration #4317. It doesn't look to be directly related but somewhat related topic so adding a link here.

One comment on this PR is that .NET loads configuration using IConfiguration generally and as you design the configuration that should be taken into account as configuration model for one language might not fit very well for how configurations are done in another language. It will be good if the mechanism for configuration gels well with that language's way of doing configuration

yurishkuro commented 1 year ago

should be taken into account as configuration model for one language might not fit very well for how configurations are done in another language.

@dpk8 can you please be more specific about where you see the divergence / difficulty? I am having a hard time making your comment actionable.

dpk83 commented 1 year ago

should be taken into account as configuration model for one language might not fit very well for how configurations are done in another language.

@DPK8 can you please be more specific about where you see the divergence / difficulty? I am having a hard time making your comment actionable.

If it's already taking that into account, great!

pellared commented 1 year ago

should be taken into account as configuration model for one language might not fit very well for how configurations are done in another language.

@DPK8 can you please be more specific about where you see the divergence / difficulty? I am having a hard time making your comment actionable.

.NET has a standard/idiomatic abstraction for dealing with configuration. See: https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.configuration

I guess the idea is that the .NET implementation should be compatible with the API of Microsoft.Extensions.Configuration. CC @open-telemetry/dotnet-maintainers

yurishkuro commented 1 year ago

@pellared you're asking everyone not familiar with .net to go and read a huge spec and make conclusions? If you or someone else has domain expertise in that area, please provide concrete concerns with the OTEP and they can be addressed. We cannot have the OTEP just say "do whatever your ecosystem does for configs", as that would make configs non-portable.

pellared commented 1 year ago

@yurishkuro The concern is about the "internal API" which states

In order to provide a minimal API surface area, implementations MUST support the following:

Does the OTEP requires to having the API with exact names/abstractions? This is something which I am not sure and is not clear to me.

I have no concerns regarding the schema and overall design.

If you or someone else has domain expertise in that area

That is why I CC'ed @open-telemetry/dotnet-maintainers to double-check 🤷

yurishkuro commented 1 year ago

Does the OTEP requires to having the API with exact names/abstractions? This is something which I am not sure and is not clear to me.

No, I don't think it's required if a similar native mechanism exists. At the same time, I think nothing prevents this:

class SDKConfiguration extends dotnet.IConfiguration {...}

func Parse(filename string) SDKConfiguration {
    dotnet.NativeParseMechanism(filename)
}

That's why we need domain experts to chime in. It's not realistic to expect PR author to go and learn all native mechanisms.

carlosalberto commented 1 year ago

@open-telemetry/specs-approvers We have reached enough approvals for this PR (after extensive feedback cycles). Please review it, and otherwise I will merge it in the next couple of days.

carlosalberto commented 1 year ago

We got a pair of comments in the last days, which are covered in the "Further Discussion", so we are aware we need tuning there. Otherwise we are good to go.