update opentelemetry SDK to 1.11.2 #138
Merged
+385
−315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
January 20, 2023 15:19
Build is broken at this point. Time to fix up the changes in OTel.
Updates all callsites and variable names to keep up with OTel.
The OTel spec does not support grpc:// or any way to deterministically demand gRPC via the endpoint URI, preferring OTEL_EXPORTER_PROTOCOL to do so. This is awkward for otel-cli so we break with the spec. otel-cli will only resolve http(s):// to HTTP protocols, defaults bare host:port to gRPC, and supports grpc:// to definitely use gRPC to connect out. This commit adds grpc:// support while working out snags around URI handling. In particular, subprocesses of otel-cli will no longer see otel-cli configuration envvars in their environment, because they are deleted at config time. This might be a little annoying when otel-cli is chained, e.g. `otel-cli exec -- otel-cli status` but that seems to be a rare usecase that can also use `otel-cli --endpoint`. It /might/ break some folks using otel-cli to execute subtasks that also want to access the OTel envvars. My guess is that this is rare to nonexistent. It might not be a bad idea to add an option to propagate envvar configs through to subprocesses, but I'm going to wait for someone to ask.
Hoping this clears an error with one of its dependencies.
This reverts commit af79dec.
This was referenced Jan 23, 2023
tobert
commented
Jan 26, 2023
| // gRPC via the endpoint URI, preferring OTEL_EXPORTER_PROTOCOL to do so. This is | ||
| // awkward for otel-cli so we break with the spec. otel-cli will only resolve | ||
| // http(s):// to HTTP protocols, defaults bare host:port to gRPC, and supports | ||
| // grpc:// to definitely use gRPC to connect out. |
There was a problem hiding this comment.
This func didn't change much except for this part. I was annoyed it was below the helpers it calls (they were added in a contributed PR a while ago) so I moved it back to the top.
There was a problem hiding this comment.
looks like no functional change at all, just added the above comment and moved the function
|
|
||
| * bare `host:port` endpoints are assumed to be gRPC | ||
| * `grpc://` URIs are supported | ||
| * `http://` and `https://` are assumed to be HTTP _only_ (the spec reuses them for gRPC) |
There was a problem hiding this comment.
Can you please confirm that it's still about OTLP/gRPC and not OTLP/HTTP protocol (see OTEL_EXPORTER_OTLP_PROTOCOL).
I'm referring to the exporter specification https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#configuration-options
edw-eqix
approved these changes
Jan 26, 2023
| @@ -65,6 +66,7 @@ func (cs *Server) ServeGPRC(listener net.Listener) error { | |||
| // ListenAndServeGRPC starts a TCP listener then starts the GRPC server using | |||
| // ServeGRPC for you. | |||
| func (cs *Server) ListenAndServeGPRC(otlpEndpoint string) { | |||
| otlpEndpoint = strings.TrimPrefix(otlpEndpoint, "grpc://") | |||
There was a problem hiding this comment.
might be worth a comment here mentioning the docs/spec
| // gRPC via the endpoint URI, preferring OTEL_EXPORTER_PROTOCOL to do so. This is | ||
| // awkward for otel-cli so we break with the spec. otel-cli will only resolve | ||
| // http(s):// to HTTP protocols, defaults bare host:port to gRPC, and supports | ||
| // grpc:// to definitely use gRPC to connect out. |
There was a problem hiding this comment.
looks like no functional change at all, just added the above comment and moved the function
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the OTel SDK to 1.11.2.
This upgrade forced a couple changes that need thinking through.
grpc://URI for deterministically asking for gRPC in an endpoint config (not in the spec)http://orhttps://URIs (spec allows this, otel-cli does not)Solves: #136
Supercedes: #113 #131 #132 #133
Beyond that, I reordered functions in plumbing.go so initTracer() is at the top again and added godoc to the helpers. Also beefed up localhost detection a touch to avoid a lookup for
localhost,127.0.0.1, and::1. Also adjusted tests to not rely on config envvars showing up inotel-cli statusorotel-cli exec's children.