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

[chore] add integration test #3677

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jan 28, 2025

Relates to #3662

This PR adds a new integration test that builds the webhook, the java autoinstrumentation image, deploys both alongside Tomcat in a kind cluster and checks from the collector that Tomcat logs are captured.

This test is a start to show how we can build integration tests for this product. This type of test will help identify regressions.

@atoulme atoulme marked this pull request as ready for review January 28, 2025 20:53
@atoulme atoulme requested a review from a team as a code owner January 28, 2025 20:53
@swiatekm
Copy link
Contributor

I really like the concept of this test! Using the debug exporter and looking at otel container logs is simple, and lets us verify the SDK is emitting logs, if not their full content and metadata.

That said, this PR essentially reimplements a lot of what our e2e test framework already does, like:

  1. Starting kind
  2. Installing the operator and its CRDs
  3. Waiting until the operator is ready
  4. Certificates, in our e2e test we just install cert-manager
  5. We should use the operator to create our collector
  6. Probably some other stuff I missed

So I'd love this test to be reimagined as a chainsaw test similar to https://github.com/open-telemetry/opentelemetry-operator/tree/main/tests/e2e-openshift/otlp-metrics-traces. It's for openshift, but it shows how you can include a bash script in the test to verify that you see the logs you expect.

This way, the test is easy to run locally as well.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 30, 2025

Alright, so you do have integration tests! (sorry I didn't see them back then)

There's a bit of triage here, we can split this into more specific tickets:

  • Certificates, in our e2e test we just install cert-manager
    sounds like we would want to test for CSRs too then. Is that not covered in tests?
  • We should use the operator to create our collector
    I am not sure why that's a requirement, typically best to test just one thing well. Are there any tests to check for autoinstrumentation?
  • existing e2e chainsaw tests
    Your test is for Openshift indeed, and so leverages a set of Openshift specific utilities. Is there any work towards testing vanilla kubernetes? Is there any way to reuse the existing chainsaw tests to test vanilla kubernetes?
  • the test is easy to run locally
    Is that a requirement? It's not hard to run this test locally by following what the yaml says, but I can certainly integrate this type of testing into a makefile. I have not been able to run e2e tests locally.

@swiatekm
Copy link
Contributor

swiatekm commented Jan 31, 2025

* Certificates, in our e2e test we just install cert-manager
  sounds like we would want to test for CSRs too then. Is that not covered in tests?

Strictly speaking, we require cert-manager right now. CSRs do work, but we have neither documentation nor tests involving them. If you'd like to add that, it should definitely be a separate issue.

* We should use the operator to create our collector
  I am not sure why that's a requirement, typically best to test just one thing well. Are there any tests to check for autoinstrumentation?

It's not a requirement, but the operator does a bunch of bookeeping for us, so the manifest is smaller. For autoinstrumentation, see https://github.com/open-telemetry/opentelemetry-operator/tree/main/tests/e2e-instrumentation. These tests don't look at emitted data though, just that injection was successful and set the right environment variables, and that the instrumented application started successfully afterwards.

* existing e2e chainsaw tests
  Your test is for Openshift indeed, and so leverages a set of Openshift specific utilities. Is there any work towards testing vanilla kubernetes? Is there any way to reuse the existing chainsaw tests to test vanilla kubernetes?

That's just an example to show how you can use a custom bash script in a chainsaw test. The majority of our tests don't have anything to do with openshift and run in kind without issue.

* the test is easy to run locally
  Is that a requirement? It's not hard to run this test locally by following what the yaml says, but I can certainly integrate this type of testing into a makefile. I have not been able to run e2e tests locally.

I consider it a requirement. Do you encounter any issues running make prepare-e2e e2e? The only requirements are docker and Go.

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.

2 participants