Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add configuration for client TLS auth and rename variable for trusted… #1375

Closed
wants to merge 7 commits into from
+5 −1
Closed

Add configuration for client TLS auth and rename variable for trusted… #1375

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a51be18
Add configuration for client TLS auth and rename variable for trusted…
Jan 26, 2021
e3a228c
Update specification/protocol/exporter.md
Jan 26, 2021
e5c1f7f
New names
Jan 27, 2021
438e536
Merge branch 'client-tls' of github.com:anuraaga/opentelemetry-specif…
Jan 27, 2021
24d9fa0
Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…
Jan 28, 2021
9caecbb
Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…
Feb 5, 2021
e831e39
Update changelog
Feb 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion specification/protocol/exporter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ The following configuration options MUST be available to configure the OTLP expo
| -------------------- | ------------------------------------------------------------ | ----------------- | ------------------------------------------------------------ |
| Endpoint | Target to which the exporter is going to send spans or metrics. The endpoint MUST be a valid URL with scheme (http or https) and host, and MAY contain a port and path. A scheme of https indicates a secure connection. When using `OTEL_EXPORTER_ENDPOINT` with OTLP/HTTP, exporters SHOULD follow the collector convention of appending the version and signal to the path (e.g. `v1/traces` or `v1/metrics`). The per-signal endpoint configuration options take precedence and can be used to override this behavior. See the [OTLP Specification][otlphttp-req] for more details. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_SPAN_ENDPOINT` `OTEL_EXPORTER_OTLP_METRIC_ENDPOINT` |
| Protocol | The protocol used to transmit the data. One of `grpc`,`http/json`,`http/protobuf`. | `grpc` | `OTEL_EXPORTER_OTLP_PROTOCOL` `OTEL_EXPORTER_OTLP_SPAN_PROTOCOL` `OTEL_EXPORTER_OTLP_METRIC_PROTOCOL` |
| Certificate File | Path to certificate file for TLS credentials of gRPC client. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_CERTIFICATE` |
| Client certificate / key | The TLS certificate and private key to authenticate the client for mTLS. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_TLS_CERTIFICATE`, `OTEL_EXPORTER_OTLP_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_SPAN_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_METRIC_TLS_PRIVATE_KEY` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a usage of this variable, example here: https://github.com/open-telemetry/opentelemetry-python/blob/db74594d93b7e42d26ed2061b7672d1ad823f30c/exporter/opentelemetry-exporter-otlp/tests/test_otlp_metric_exporter.py#L74 Can you please make sure to check that this is fine to change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be renamed to OTEL_EXPORTER_OTLP_METRIC_TLS_TRUSTED_CERTIFICATE I think. Is that what you'd like to confirm?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we still allow the old Environment variable but mark it deprecated with a migration?

Given how CLOSE we are to 1.0 I think it'd be good if we pretend we have users we don't want to break, to get in the habit of how these changes will have to be made going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If languages want to support multiple variables I think they could, even without it being mentioned in the spec right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment was about checking with python maintainers whether it is already heavily in use. As @jsuereth pointed we need to be careful with renaming. If not much adoption so far, PR good with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there aren't any open source usage

https://github.com/search?q=OTEL_EXPORTER_OTLP_METRIC_CERTIFICATE&type=code

And if we were done making changes, we'd already be 1.0 - this doc doesn't have any label on it, not even feature-freeze like some of the other ones so it seems a bit unfair to require too much process. The variable seems to be a bug really since the description doesn't describe it, these are credentials of a server, not a client.

Languages can use a deprecation cycle to change the variable if they wish, but I don't think that's something to have here, or if we did, it would be a much more general document detailing a mandate for and how to deprecate features.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not asking for much process. Maybe just a ack from @open-telemetry/python-approvers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok - @open-telemetry/python-approvers does this change look ok? Thanks!

| Trusted certificate | The trusted certificate to use when verifying a server's TLS credentials. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_TLS_TRUSTED_CERTIFICATE` |
| Headers | Key-value pairs to be used as headers associated with gRPC or HTTP requests. See [Specifying headers](./exporter.md#specifying-headers-via-environment-variables) for more details. | n/a | `OTEL_EXPORTER_OTLP_HEADERS` `OTEL_EXPORTER_OTLP_SPAN_HEADERS` `OTEL_EXPORTER_OTLP_METRIC_HEADERS` |
| Compression | Compression key for supported compression types. Supported compression: `gzip`| No value | `OTEL_EXPORTER_OTLP_COMPRESSION` `OTEL_EXPORTER_OTLP_SPAN_COMPRESSION` `OTEL_EXPORTER_OTLP_METRIC_COMPRESSION` |
| Timeout | Max waiting time for the backend to process each spans or metrics batch. | 10s | `OTEL_EXPORTER_OTLP_TIMEOUT` `OTEL_EXPORTER_OTLP_SPAN_TIMEOUT` `OTEL_EXPORTER_OTLP_METRIC_TIMEOUT` |
Expand Down