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.

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

Add fixture-based e2e tests with sample apps #46

Merged
merged 9 commits into from
Apr 3, 2023

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Mar 24, 2023

Fixes #39

This adds an e2e test matrix (largely based on collector-contrib's tests) that does the following:

  1. Builds the auto-instrumentation agent, launcher (to be removed with Allocate memory without launcher #40), and sample apps using each supported library
  2. Spins up a kind cluster
  3. Starts an otel collector with trace receiver and file exporter
  4. Starts the sample app for that library
  5. The sample app calls itself to generate trace data
  6. Copies the file exporter output to a local fixture
  7. Checks for changes to the current fixture, ignoring/removing timestamps

I used a k8s cluster instead of just running the binaries for a couple reasons: First, I wanted to just use a collector image from upstream rather than building it ourselves. Second, k8s gives some nice orchestration around starting up and stopping these processes that works well as a Github action. I couldn't find a good way to build local binaries and have those run in the background, while moving on to the next Workflow step, and checking that everything had completed successfully.

The steps are basically copied to a new make fixture-$LIBRARY target that lets you generate these fixtures. Eg, make fixture-nethttp will run all the steps from above. Failures in these tests could indicate a legitimate update to the fixtures, so updating them locally will be useful.

I added sample apps for gorillamux and net/http. If someone else wants to add a gRPC app, I think the first 2 serve as a good example. New libraries should be able to drop in similarly.

Makefile Outdated Show resolved Hide resolved
.github/workflows/kind.yml Outdated Show resolved Hide resolved
test/e2e/gorillamux/Dockerfile Outdated Show resolved Hide resolved
test/e2e/nethttp/Dockerfile Outdated Show resolved Hide resolved
test/e2e/gorillamux/traces.json Outdated Show resolved Hide resolved
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

This is a comment rather for future improvment. I do not want to block this PR as we need to start with something 😉

Based on my experience in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation (where I am a maintainer) I find testing against a "test fake collector" makes the tests more maintainable:

  • spinning up a a httptest server or by setting up a gRPC server in tests is faster than running a real collector [image]
  • you get more flexibility in assertion logic

Here you can find an example implementation and usage of a "test fake collector" in Go: https://github.com/signalfx/splunk-otel-go/blob/main/distro/otel_test.go

@damemi
Copy link
Contributor Author

damemi commented Mar 29, 2023

@pellared that's a great suggestion, but I agree on not blocking this PR right now (especially because I want to unblock #40). I'll make a new issue from your comment to track that for a refactor in the future.

Right now I just want to figure out the empty labels from above which I'm trying a couple things for. Maybe an issue with context propagation from the request?

damemi and others added 5 commits March 31, 2023 20:53
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
cilium/ebpf v0.9.1 updated the Offset field to be relative, with Address being absolute.
This broke auto-instrumentation as currently written.
@damemi
Copy link
Contributor Author

damemi commented Mar 31, 2023

It looks like #55 broke auto-instrumentation due to a change in v0.9.1 that updated UprobeOptions.Offset to be relative, with Address now being absolute. Updated this in dddbb7e and running e2es locally now seem to work again, hopefully they pass in CI too

@damemi
Copy link
Contributor Author

damemi commented Apr 3, 2023

They're green! 🎉 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CI with a sample app to check that changes are valid
4 participants