-
Notifications
You must be signed in to change notification settings - Fork 440
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
Actually e2e test java #3381
base: main
Are you sure you want to change the base?
Actually e2e test java #3381
Conversation
@@ -9,11 +9,9 @@ spec: | |||
- name: OTEL_EXPORTER_OTLP_ENDPOINT | |||
value: http://localhost:4317 | |||
- name: OTEL_EXPORTER_OTLP_TIMEOUT | |||
value: "20" | |||
value: "20000" |
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.
this is to account for the collector not being ready fast enough
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.
What happens if it's not ready? Shouldn't this work correctly either way?
value: parentbased_traceidratio | ||
- name: OTEL_TRACES_SAMPLER_ARG | ||
value: "0.85" | ||
value: always_on |
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.
don't want our test data to be sampled
@@ -41,3 +41,6 @@ spec: | |||
traces: | |||
receivers: [otlp] | |||
exporters: [debug] | |||
metrics: |
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.
i was going to attempt the TLS test, but it seemed way too hard to do on a first-pass
@@ -20,3 +20,6 @@ spec: | |||
traces: | |||
receivers: [otlp] | |||
exporters: [debug] | |||
metrics: |
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.
java auto inst can send metrics too, why not accept them :)
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.
+1
@@ -473,7 +473,7 @@ KUSTOMIZE_VERSION ?= v5.0.3 | |||
CONTROLLER_TOOLS_VERSION ?= v0.16.1 | |||
GOLANGCI_LINT_VERSION ?= v1.57.2 | |||
KIND_VERSION ?= v0.20.0 | |||
CHAINSAW_VERSION ?= v0.2.8 | |||
CHAINSAW_VERSION ?= v0.2.11 |
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.
this version does not work on go 1.22. I would wait with updating before we upgrade the go version
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.
If that's a concern for us, we should install it by downloading the release binary instead. Our supported Go version shouldn't get in the way of tooling upgrades.
@@ -20,3 +20,6 @@ spec: | |||
traces: | |||
receivers: [otlp] | |||
exporters: [debug] | |||
metrics: |
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.
+1
#!/bin/bash | ||
# set -ex | ||
pod=$(kubectl get pods -n $NAMESPACE | awk '{print $1}' | tail -n 1) | ||
kubectl get --raw /api/v1/namespaces/$NAMESPACE/pods/${pod}:8080/proxy/ |
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.
this is new to me :)
@IshwarKanse does it work on OCP?
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.
Looking at this, I really wonder if we shouldn't use e2e_framework and/or terratest for these tests. The main problem is really getting data out of the cluster, as we don't have a remote we can easily query. I can think of a few solutions to do this in kind, but they won't work on OpenShift. It'd be best if these could simply be Go tests we could run on a host against any cluster.
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.
This can be implemented natively using Chainsaw
and avoid using the script. https://github.com/kyverno/chainsaw/blob/main/testdata/e2e/examples/proxy/chainsaw-test.yaml
We were also discussing on implementing some kind of custom store in Chainsaw which gathers all the signals and would be useful for asserting. https://docs.google.com/document/d/1G4uDMrC3okF2q2qyNYTg2qj7yxNBjSTGgn8pBEXEeiw/edit#heading=h.8mm7ay5rgjs6
@eddycharly Do we have any decision on what custom store we can use for testing observability data. ?
Description:
Uses chainsaw to seed and then check for data in the e2e test for the java instrumentation only right now
Link to tracking Issue(s): #552
Testing:
Documentation: