-
Notifications
You must be signed in to change notification settings - Fork 24
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 support for monitor tab in Jaeger console #470
Conversation
Codecov Report
@@ Coverage Diff @@
## main #470 +/- ##
==========================================
+ Coverage 78.11% 78.22% +0.10%
==========================================
Files 64 64
Lines 4707 4758 +51
==========================================
+ Hits 3677 3722 +45
- Misses 857 861 +4
- Partials 173 175 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Value: tempo.Spec.Template.QueryFrontend.JaegerQuery.MonitorTab.PrometheusEndpoint, | ||
}, | ||
}, | ||
Args: []string{ |
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 need to add a condition here to set the TLS/token file only when deployed on OCP
@@ -0,0 +1,13 @@ | |||
#!/bin/bash |
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.
Could you move this shellscript to a container? This will give us more deterministic results (e.g. curl might not be installed on every test host, may have a different version, etc.)
Something like here: https://github.com/grafana/tempo-operator/blob/3b402384c9e78b4cd0ae075548f4c6cebab82505/tests/e2e/smoketest-with-jaeger/03-verify-traces.yaml
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 I will change it
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.
Looks great!
Do we plan to support this also with upstream Kubernetes?
If so, should we add an e2e test for the upstream suite?
f39c775
to
9057fc5
Compare
I am seeing the following error even when a bundle is provisioned from annotation:
|
423a3cf
to
e1313c9
Compare
The e2e test is passing now locally fine (I did 4 runs). However the solution does not work with OTEL collector 0.80.0 and above. Once Tempo 2.2 is released I will be able to support it via jaegertracing/jaeger#4555 |
} | ||
// If the endpoint matches Prometheus on OpenShift, configure TLS and token based auth | ||
prometheusEndpoint := strings.TrimSpace(tempo.Spec.Template.QueryFrontend.JaegerQuery.MonitorTab.PrometheusEndpoint) | ||
if prometheusEndpoint == "https://thanos-querier.openshift-monitoring.svc.cluster.local:9091" { |
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.
imho these settings (prometheus.tls.enabled
, prometheus.token-file
, prometheus.tls.ca
) should be part of the monitorTab
field in the CR, otherwise this feature will only work on non-tls prometheus and the monitoring stack on OpenShift.
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.
and to keep it simple for users, set these new fields of the CR in the defaulter webhook to the values here, if the endpoint matches the thanos endpoint
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.
The new red-metrics
e2e test also passes for me.
Great work!
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
e1313c9
to
9762bee
Compare
@andreasgerstmayr the PR is ready to be reviewed e2e test is passing
|
Resolves #466
Based on https://docs.google.com/document/d/17TC4VPaRgK1SeP9JNFlYk17gGjJ1dtQh1ENa9wO9rVw/edit#heading=h.rqzf9iedmg7o
Blocked by #525
Depends on:
flag: --prometheus.token-file
Todos