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.
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
trace: add OTEL initialization #10526
Conversation
|
||
const maxUnixSocketPathSize = len(syscall.RawSockaddrUnix{}.Path) | ||
|
||
func DialInMemory(ctx context.Context, addr string) (net.Conn, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thaJeztah I was thinking of PRing this to docker/go-connections
later. Thoughts? It's inspired by the InmemSocket
there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@milas hmm. need to think about this; honest answer;
- there's an issue with the latest (main) version of
go-connections
that we still need to sort out (moby started failing when we tried to update it to latest) - there's been some discussion about that repository (mainly around the TLS config specified in that repository, but also because it became a bit of a grab-bag of utilities that are not very related), and we need to re-think if we want to preserve it.
I wonder if we should have something like https://github.com/docker/go-metrics (either to provide abstractions, or to have a shared implementation for some of this).
(all just quick blurbs from my side! haven't dug in deep into all of this, but perhaps something we should discuss with more people)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh sorry, I should have clarified -- I was only referring to the DialInMemory
function - the way that library is structured right now, you can:
- create a listener type that works across OSes (impls behind build tags)
- configure an HTTP transport across OSes
...but it does NOT expose a function to create a platform-agnostic in-memory net.Conn
directly
(That said, yes -- the rest of the observability pieces should be shared! I purposefully put this in internal/tracing
so it cannot become a dependency elsewhere & it doesn't depend on anything in the rest of the Compose code base. Will spin it out into its own repo/module later)
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## v2 #10526 +/- ##
==========================================
- Coverage 59.65% 59.01% -0.64%
==========================================
Files 107 111 +4
Lines 9515 9709 +194
==========================================
+ Hits 5676 5730 +54
- Misses 3257 3395 +138
- Partials 582 584 +2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the new files need the Copyright header.
e2e tests checking the console output seem to not be happy, maybe there is a trailing character added by the otel configuration? 🤔
39eae89
to
368e242
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a bunch of OTEL initialization code. It's all in `internal/` because there are re-usable parts here, but Compose isn't the right spot. Once we've stabilized the interfaces a bit and the need arises, we can move it to a separate module. Currently, a single span is produced to wrap the root Compose command. Compose will respect the standard OTEL environment variables as well as OTEL metadata from the Docker context. Both can be used simultaneously. The latter is intended for local system integration and is restricted to Unix sockets / named pipes. None of this is enabled by default. It requires setting the `COMPOSE_EXPERIMENTAL_OTEL=1` environment variable to gate it during development. Signed-off-by: Milas Bowman <milas.bowman@docker.com>
What I did
This is a bunch of OTEL initialization code. It's all in
internal/
because there are re-usable parts here, but Compose isn't the right spot. Once we've stabilized the interfaces a bit and the need arises, we can move it to a separate module.Currently, a single span is produced to wrap the root Compose command.
Compose will respect the standard OTEL environment variables as well as OTEL metadata from the Docker context. Both can be used simultaneously. The latter is intended for local system integration and is restricted to Unix sockets / named pipes.
None of this is enabled by default. It requires setting the
COMPOSE_EXPERIMENTAL_OTEL=1
environment variable to gate it during development.(not mandatory) A picture of a cute animal, if possible in relation to what you did