Add test to otelhttp to validate semantic conventions #4701
Conversation
| wd, err := os.Getwd() | ||
| ctx := context.Background() | ||
| req := testcontainers.ContainerRequest{ | ||
| Image: "ghcr.io/madvikinggod/semantic-convention-checker:0.0.1", |
There was a problem hiding this comment.
I see that https://github.com/MadVikingGod/otel-semconv-checker is a Go application.
How about simply running the HTTP server in the same process (reuse Go code) instead of running as a docker container?
There was a problem hiding this comment.
I've thought about this a bit, and I don't think it's a good idea.
First, that means putting almost all of the same logic that is in the main executable, config parsing, parsing the semantic conventions, and setting up the grpc server into every test that launches it directly.
Second, that means instead of taking a dependency on the config format not breaking (that is what would change between versions), we take a direct dependency on the API of the server. While eventually, that might be an acceptable idea, right now, I've been working hard to add features, and there is a lot of flux that could be problematic if you rely on the internal API vs. the docker images.
Currently, I've been using this to help design the switch to semconv v1.24.0 with the environment variable, and upgrading the docker version is a much less painful experience than aligning the config.
instrumentation/net/http/otelhttp/test/semantic_convention_test.go
Outdated
Show resolved
Hide resolved
| l.t.Log(string(log.Content)) | ||
| } | ||
|
|
||
| var docker = flag.Bool("docker", false, "Enable tests that use docker.") |
There was a problem hiding this comment.
Should there be a specific test suite running this in CI? Or do you plan this for later?
Also, an alternative here rather than use the flag package could be to use a // go:build docker build flag so the file doesn't run by default, but can be enabled.
There was a problem hiding this comment.
// go:build docker
Let's not do that. Many tools (IDE, golangci-lint) might stop working.
However, I think we could (should) enable it by default.
We can consider skipping when -short is provided using this pattern: https://pkg.go.dev/testing#hdr-Skipping.
There was a problem hiding this comment.
I made it part of the non--short tests. I do want to warn that this makes docker required for running all tests. including make test and make precommit
There was a problem hiding this comment.
Can I resolve this?
It will be skipped on short tests.
I want to start a conversation about semantic conventions and how we can better validate instrumentation outputs.
I'm looking for some feedback on this approach and what I need to do to get this as the pattern for validating most/all of our instrumentation.
This PR here adds a test in the otelhttp package that uses testcontainers to run
https://github.com/MadVikingGod/otel-semconv-checkerand capture the output from a wrapped HTTP client and server.This has a hard dependency on docker, which is why I put the test behind a
-dockerflag.How I would see this used in other instrumentation is a similar testcontiner setup, but different config, gprc wouldn't have the same attribute groups, and a different server/client wrap setup.