-
Notifications
You must be signed in to change notification settings - Fork 30
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
tempo-query: bump to version with separate tls settings #1057
Conversation
80a2de7
to
73a9d77
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1057 +/- ##
=======================================
Coverage 69.14% 69.14%
=======================================
Files 110 110
Lines 7059 7059
=======================================
Hits 4881 4881
Misses 1888 1888
Partials 290 290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
73a9d77
to
d5780d9
Compare
Makefile
Outdated
@@ -12,7 +12,7 @@ MIN_OPENSHIFT_VERSION ?= 4.12 | |||
|
|||
TEMPO_IMAGE ?= docker.io/grafana/tempo:$(TEMPO_VERSION) | |||
JAEGER_QUERY_IMAGE ?= docker.io/jaegertracing/jaeger-query:$(JAEGER_QUERY_VERSION) | |||
TEMPO_QUERY_IMAGE ?= docker.io/grafana/tempo-query:$(TEMPO_QUERY_VERSION) | |||
TEMPO_QUERY_IMAGE ?= ghcr.io/frzifus/tempo-query:$(TEMPO_QUERY_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.
I'm not sure if we should start linking to private repos, maybe we should wait until it's merged upstream or link to the os-observability
org on GitHub?
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.
It actually got merged today. I will wait for the next nightly build and replace it :)
@@ -1,5 +1,5 @@ | |||
address: 127.0.0.1:7777 | |||
backend: 127.0.0.1:{{ .HTTPPort }} | |||
address: 0.0.0.0:7777 |
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.
Jaeger Query and Tempo Query run in the same pod, sharing the network stack, why can't they communicate over localhost?
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 health check does use the $POD_IP.
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.
Ah, right. Could you add a comment ("required for health check")? Otherwise I'll wonder about the same thing again next time I read the code :D
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.
Sure :)
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
d5780d9
to
85f3258
Compare
meeeh..
|
This PR fixes the failing
multitenancy
tests.$ chainsaw test --config .chainsaw-openshift.yaml tests/e2e-openshift/multitenancy --- PASS: chainsaw (0.00s) --- PASS: chainsaw/multitenancy (240.82s) PASS Tests Summary... - Passed tests 1 - Failed tests 0 - Skipped tests 0 Done.
Full details
Output
While observing the test, I noticed that the tempo container crashed from time to time. I could not determine exactly why this is the case.
Image: grafana/tempo#4177