open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.64k stars 870 forks source link

If protocol suffix is alloed for Grpc default value https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/ #4077

Closed michaelkira closed 1 week ago

michaelkira commented 3 weeks ago

What needs to be changed? Describe the update that is required. In https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/, the default value for gRPC is with protocol suffix But such endpoint is dont accepted by telegraf, more details can be found in this issue https://github.com/influxdata/telegraf/issues/15450

Please review protocol suffix is allowed(or disallowed) for the gRPC service

What is the name + path of the page that needs changed? The relative path and page title where you found a problem. If protocol suffix is alloed for Grpc default value https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/

Additional context: anything else you can think about that would be helpful.

theletterf commented 3 weeks ago

@open-telemetry/collector-approvers Is this something that would need to be changed?

mx-psi commented 3 weeks ago

I don't think this belongs to the Collector SIG, this is about the SDKs OTLP exporter

cartermp commented 2 weeks ago

I'm not sure if this is a bug, but I do think it's probably worth changing. The spec also uses http in examples for gRPC endpoints, but I don't know if that was intentional or not, and that's what the SDK config docs are based on.

svrnm commented 2 weeks ago

This is a spec issue, not a documentation issue, since as @cartermp stated we follow the spec:

https://opentelemetry.io/docs/specs/otel/protocol/exporter/#configuration-options

Endpoint (OTLP/gRPC): Target to which the exporter is going to send spans, metrics, or logs. The option SHOULD accept any form allowed by the underlying gRPC client implementation. Additionally, the option MUST accept a URL with a scheme of either http or https. A scheme of https indicates a secure connection and takes precedence over the insecure configuration setting. A scheme of http indicates an insecure connection and takes precedence over the insecure configuration setting. If the gRPC client implementation does not support an endpoint with a scheme of http or https then the endpoint SHOULD be transformed to the most sensible format for that implementation.

@michaelkira please take a look, if you still think this needs to be addressed I am happy to transfer this issue to the spec repo for you

michaelkira commented 2 weeks ago

@svrnm Thanks, please help transfer. I think this worth addressing or at least a classification with more information. I see the spec says If the gRPC client implementation does not support an endpoint with a scheme of http or https then the endpoint SHOULD be transformed to the most sensible format for that implementation. So, who should be responsible for such transformation, the sdk? or the user of sdk? If sdk, does this mean all otel sdk should support grpc endpoint with protocol suffix. (which wont help a lot in my case, as telegraf have parsing logic before it passed into goland sdk) If the sdk user, does this mean, when construct endpoint for otel exporter, we need to clearly understand the details about gRPC client sdk used by otel sdk?

MrAlias commented 1 week ago

@michaelkira can you provide a example of your issue? It is not clear what you are doing, what is happening, and what you expect to happen.

The gRPC endpoint should be parsed and the protocol (http or https) should be stripped before it is used by any setup exporter. Are you not seeing this in an OTel component?

cartermp commented 1 week ago

@MrAlias It looks like it's Telegraf behavior: https://github.com/influxdata/telegraf/issues/15450#issuecomment-2147471525

MrAlias commented 1 week ago

It looks like this can be closed. A vendor has decided to support a different configuration protocol, there is nothing for OTel to do in this situation.

svrnm commented 1 week ago

Hey @michaelkira, I am closing this issue now based on @MrAlias comment, if you think this is not justified, please follow up