Remove the default metrics namespace/prefix for span RED metrics in Jaeger query #1072
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
frzifus
force-pushed
the
fix/red-metrics
branch
from
7a47840 to
6d08dc0
Compare
pavolloffay
reviewed
Oct 28, 2024
| change_type: bug_fix | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. tempostack, tempomonolithic, github action) | ||
| component: tempostack |
There was a problem hiding this comment.
Is the fix also fixing monolithic?
There was a problem hiding this comment.
For me it looks like that was never supported? I can not find anything about the STORAGE_METRICS_TYPE.
21054a4#diff-2dd4203d1e98f65856b756889e3b555cd1909bf0133911ad1cc7bf0667f8573bR295
╰─❯ grep -r "METRICS_STORAGE_TYPE"
grep: bin/manager: Übereinstimmungen in Binärdatei
internal/manifests/queryfrontend/query_frontend.go: Name: "METRICS_STORAGE_TYPE",
internal/manifests/queryfrontend/query_frontend_test.go: env: []corev1.EnvVar{{Name: "METRICS_STORAGE_TYPE", Value: "prometheus"}, {Name: "PROMETHEUS_SERVER_URL", Value: "http://prometheus:9091"}},
internal/manifests/queryfrontend/query_frontend_test.go: {Name: "METRICS_STORAGE_TYPE", Value: "prometheus"},
There was a problem hiding this comment.
There was a problem hiding this comment.
yes, monolithic doesn't support red metrics at the moment
frzifus
changed the title
|
@frzifus could you please paste chainsaw otput from openshift tests here? |
frzifus
force-pushed
the
fix/red-metrics
branch
from
6d08dc0 to
66fde82
Compare
Full output |
|
Any idea whats wrong with the Operator-Upgrade step? Installing it locally via olm works fine. cc @IshwarKanse |
The error is coming from this command: |
|
@andreasgerstmayr @frzifus Its an issue with OLM installation: operator-framework/operator-lifecycle-manager#3419 |
|
Added PR to pin the OLM version: #1074 |
pavolloffay
reviewed
Oct 29, 2024
pavolloffay
reviewed
Oct 29, 2024
| // NOTE: Jaeger 1.62 default namespace changed to "traces_span_metrics". | ||
| // We fallback to no namespace. | ||
| // See https://github.com/jaegertracing/jaeger/pull/6007. | ||
| "--prometheus.query.namespace=", |
There was a problem hiding this comment.
regarding #1073 (the collector defaults to traces.span.metrics namespace) we should expose this setting in the CR to allow users using the older collector with Tempo.
There was a problem hiding this comment.
@frzifus could you please write release notes for 3.4 to reflect the breaking change what we are making here openshift/openshift-docs#83770
frzifus
force-pushed
the
fix/red-metrics
branch
from
66fde82 to
c628323
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1072 +/- ##
==========================================
+ Coverage 67.89% 67.91% +0.01%
==========================================
Files 110 110
Lines 8721 8726 +5
==========================================
+ Hits 5921 5926 +5
Misses 2503 2503
Partials 297 297
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Benedikt Bongartz <[email protected]>
frzifus
force-pushed
the
fix/red-metrics
branch
2 times, most recently
from
712f32c to
1494409
Compare
pavolloffay
reviewed
Oct 29, 2024
pavolloffay
reviewed
Oct 29, 2024
frzifus
force-pushed
the
fix/red-metrics
branch
from
1494409 to
6295451
Compare
Signed-off-by: Benedikt Bongartz <[email protected]>
frzifus
force-pushed
the
fix/red-metrics
branch
from
6295451 to
e1cbd34
Compare
pavolloffay
reviewed
Oct 29, 2024
.chloggen/fix_red-metrics.yaml
Outdated
| To achieve this the Operator will set the jaeger-query `--prometheus.query.namespace=` flag. | ||
| Since Jaeger version 1.62, jaeger-query uses `traces.span.metrics` as default too. | ||
|
|
||
| Example how to overwrite the default namespace by configuring it in the CR: |
There was a problem hiding this comment.
Can we make this example for collector before 0.109.0 ? I think the majority of users will want to know this.
@frzifus did you test that redMetricsNamespace can be set to an empty string?
There was a problem hiding this comment.
I can confirm that
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoStack
metadata:
name: redmetrics
spec:
storage:
secret:
name: minio-test
type: s3
storageSize: 1Gi
template:
gateway:
enabled: false
queryFrontend:
jaegerQuery:
enabled: true
monitorTab:
enabled: true
prometheusEndpoint: https://thanos-querier.openshift-monitoring.svc.cluster.local:9091
redMetricsNamespace: ""
ingress:
type: route
Leads to an empty flag value:
- args:
- --query.base-path=/
- --span-storage.type=grpc
- --grpc-storage.server=localhost:7777
- --query.bearer-token-propagation=true
- --prometheus.query.namespace=
- --prometheus.tls.enabled=true
- --prometheus.token-file=/var/run/secrets/kubernetes.io/serviceaccount/token
- --prometheus.token-override-from-context=false
- --prometheus.tls.ca=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
env:
- name: METRICS_STORAGE_TYPE
value: prometheus
- name: PROMETHEUS_SERVER_URL
value: https://thanos-querier.openshift-monitoring.svc.cluster.local:9091
image: docker.io/jaegertracing/jaeger-query:1.62.0
imagePullPolicy: IfNotPresent
name: jaeger-query
Signed-off-by: Benedikt Bongartz <[email protected]>
frzifus
force-pushed
the
fix/red-metrics
branch
3 times, most recently
from
4039101 to
13d39da
Compare
Signed-off-by: Benedikt Bongartz <[email protected]>
Signed-off-by: Benedikt Bongartz <[email protected]>
frzifus
force-pushed
the
fix/red-metrics
branch
from
13d39da to
2dc79f4
Compare
|
pavolloffay
reviewed
Oct 30, 2024
Signed-off-by: Benedikt Bongartz <[email protected]>
frzifus
force-pushed
the
fix/red-metrics
branch
from
ba89de4 to
5d830df
Compare
pavolloffay
approved these changes
Oct 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix RED metric retrieval from prometheus by removing the in Jaeger 1.62 introducted default RED metric namespace.
The Operator overwrites the new default namespace jaeger-query takes into account using the
--prometheus.query.namespace=flag.More details about the breaking changes can be found here: