Clarify env variables in otlp exporter

[jan25]

Changes summary

• Make insecure=true as default, so supplying certificate file is not mandatory and can be supplied on setting insecure=false explicitly.
• Clarify OTEL_EXPORTER_OTLP_CERTIFICATE takes path to certificate file.
• Clarify OTEL_EXPORTER_OTLP_HEADERS take key-value pairs.

Related issues

open-telemetry/opentelemetry-python#1004 which is about implementing the exporter spec to have env variables.

Notes

We may have to update existing implementations to follow new insecure variable default. How to ensure libraries catchup with this?

[tigrannajaryan]

Make insecure=true as default, so supplying certificate file is not mandatory and can be supplied on setting insecure=false explicitly.

Why do we need to change the default for insecure?

[jan25]

Thanks for the reviews!

Hi @tigrannajaryan, insecure=false as default implies certificates will have to be supplied to work with exporter, even for a demo purposes, which didn't look natural to me, so decided to propose true default. If users decide to use TLS they could explicitly pass insecure=false along with certificates path.

Also, I think we can be more clear by having tls_enabled=true|false instead of insecure=false|true. wdyt?

[tigrannajaryan]

Hi @tigrannajaryan, insecure=false as default implies certificates will have to be supplied to work with exporter, even for a demo purposes, which didn't look natural to me, so decided to propose true default. If users decide to use TLS they could
explicitly pass insecure=false along with certificates path.

To be precise insecure=false default implies that:

• Either the insecure=true has to be passed explicitely, or
• The certificate will need to be supplied.

This does not look unnatural to me if the goal is to force the user to think whether they want it to be secure or no and act on it. The defaults are chosen in a way that if the user doesn't make the choice then it won't work at all.

With the change proposed in this PR it will no longer be the case. The user will not necessarily be deliberately making the choice anymore. They may not even be aware of the options.

I don't know which of the approaches is desirable for us (force the user to make the choice or make it easy to run in the insecure mode). The answer is is not obvious to me.

Also, I think we can be more clear by having tls_enabled=true|false instead of insecure=false|true. wdyt?

I would stay with insecure because that's what Collector config uses everywhere. It is useful to be consistent across the different parts of the project.

[iNikem]

I support the argument of @tigrannajaryan about forcing users to make the conscious choice.

[anuraaga]

Just to confirm, does insecure = false actually require setting a certificate? If the endpoint is public, meaning it has a root-signed cert, it will work without setting a certificate right?

Also from what I can tell, Java has a bug where TLS is disabled when insecure is false. Please check my work, but the positive and negative seem to be flipped right?

https://github.com/open-telemetry/opentelemetry-java/blob/master/exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java#L321

So for Java we currently set OTLP exporter to default to pointing at localhost at the well defined port without TLS. This allows quickly playing with OTel, which has works pretty nicely so far, I assumed it was intentional :) I think it's basically inconceivable for localhost to have TLS set up. So if we were to default to TLS on, I would remove the default for the endpoint - I think it's unidiomatic to have a default of localhost also have TLS default to on, it basically never works in practice.

[tigrannajaryan]

Just to confirm, does insecure = false actually require setting a certificate? If the endpoint is public, meaning it has a root-signed cert, it will work without setting a certificate right?

Supposedly yes, if the client uses its local CA-cert repository to verify the server. The spec does not define this, but I'd expect most implementations to behave like that.

[tigrannajaryan]

Also from what I can tell, Java has a bug where TLS is disabled when insecure is false. Please check my work, but the positive and negative seem to be flipped right?

https://github.com/open-telemetry/opentelemetry-java/blob/master/exporters/otlp/src/main/java/io/opentelemetry/exporters/otlp/OtlpGrpcSpanExporter.java#L321

Yes, I think the confusion is from a the name KEY_USE_TLS which has a reverse meaning while in reality it checks for insecure flag.

[tigrannajaryan]

So for Java we currently set OTLP exporter to default to pointing at localhost at the well defined port without TLS. This allows quickly playing with OTel, which has works pretty nicely so far, I assumed it was intentional :) I think it's basically inconceivable for localhost to have TLS set up. So if we were to default to TLS on, I would remove the default for the endpoint - I think it's unidiomatic to have a default of localhost also have TLS default to on, it basically never works in practice.

This is a good point.

[jan25]

I looked through few existing tools for TLS setting:

• Zipkin server has insecure=true by default, but can be enabled by setting ssl.enabled=true
• Jaeger agent has insecure=true by default, but can be enabled by setting reporter.grpc.tls.enabled=true
• OpenCensus agent library has insecure=false by default, but can be disabled by using WithInsecure()

As mentioned in previous comment, with insecure=true its very easy to get started for demo purposes like running on localhost. Having insecure=false doesn't pose big hurdle either, it can be disabled using library function or Env variable in exporter, similar to OpenCensus agent case above.

I think we can go with maintainers opinion on this. I'll revert changing the insecure=false default. And finalise other minor fixes in PR this week.

[anuraaga]

I don't know if we got a clear advice from maintainers yet. Apparently my point was a good one though :)

Even in production, in sidecar deployment, it's really rare to use TLS. I think a default endpoint of localhost should go with a default security of no-tls, it's either for a demo, or for a sidecar. That being said, it could make sense to change the default in another PR instead of this one (I'll be a bit said in the meantime to update Java to be compliant though, adding the flag adds complexity when explaining OTel to a new user 😿 )

[tigrannajaryan]

I don't have a strong opinion on what should be the default. There are cons and pros for either approach. However, I do want to see a wider discussion before accepting the change.

[tigrannajaryan]

LGTM in current form.

[jan25]

@tigrannajaryan thanks! I've opened an issue #1002 to further discuss on insecure mode defaults

[tigrannajaryan]

We can merge this today or after spec freeze. I consider this an editorial change so it is OK to merge it after.

[arminru]

I also consider this an editorial change since #1002 was carved out.