[tempo-distributed] drop jaeger-ui related ports and config completely #3963
Conversation
This file contains hidden or 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
…o-query only if enabled Currently the service monitor is added regardless of whether the old jaeger mode is enabled or not. Similar to the ports on the deployment and service, we only include the servicemonitor if `queryFrontend.query.enabled=true`. Additionally, the selector of the service monitor does not work anyway, because the service that includes the old ports has a different `app.kubernetes.io/component` label than the one created from the servicemonitor template Signed-off-by: Alexander Bartolomey <[email protected]>
1. remove servicemonitor. tempo-query is now only a gRPC proxy for jaeger. There's not metrics, and no UI anymore 2. remove jaeger metrics and ui ports from service. They plainly dont exist anymore. the ports are also not what the server expects, there's a single server running on default 7777 3. use correct config file for tempo-query, the executable loads the wrong config file Signed-off-by: Alexander Bartolomey <[email protected]>
The port was still pointing to a jaeger-related HTTP port, this is gone since grafana/tempo#3840 so don't use it anymore HTTP ingress is no longer for connecting to jaeger UI, but can instead be used for e.g. grafana data sources access via ingress Signed-off-by: Alexander Bartolomey <[email protected]>
zoomoid
requested review from
a team,
BitProcessor,
QuentinBisson,
Sheikh-Abubaker,
faustodavid and
swartz-k
as code owners
zoomoid
changed the title
Signed-off-by: Alexander Bartolomey <[email protected]>
| @@ -1,4 +1,4 @@ | |||
| {{- if and .Values.queryFrontend.query.enabled .Values.queryFrontend.ingress.enabled -}} | |||
| {{- if .Values.queryFrontend.ingress.enabled -}} | |||
Collaborator
There was a problem hiding this comment.
Is there any reason to change this?
| service: | ||
| # -- Port of the query-frontend service | ||
| port: 16686 | ||
| port: 3200 |
Collaborator
There was a problem hiding this comment.
Should it not be 7777?
Collaborator
There was a problem hiding this comment.
If it should, can you use this value in the templates instead to avoid magic numbers?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This no longer works, grafana/tempo#3840 removed jaeger from tempo-query, changing with it the port that the server running in the container listens to:
7777of tempo-query. Note that the container port isn't configurable right now. If you opt to setADDRESSviaqueryFrontend.query.extraEnvsto something different, you cannot access the server anymore from outside the pod.queryFrontendIngress. This no longer works for accessing Jaeger UI. Instead, you can repurpose the ingress, e.g. for configuring Grafana data sourcestempo-querydoesn't export any metrics, there's nothing to scrape there, so this can be dropped completely in favour of the already existing service monitor for query-frontend's HTTP portmost of these changes probably also need to be reflected in the non-distributed charts as well, but I'm awaiting feedback here first.