-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[tempo-distributed] Add appProtocol to Enable Tempo Streaming #3903
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
base: main
Are you sure you want to change the base?
Conversation
Sheikh-Abubaker
left a comment
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.
@msgSend I believe appProtocol is already defined here for both the case could you please checkout if it works the same ?
query-frontend:
helm-charts/charts/tempo-distributed/templates/query-frontend/service-query-frontend.yaml
Lines 27 to 29 in 76d7026
| {{- if .Values.queryFrontend.appProtocol.grpc }} | |
| appProtocol: {{ .Values.queryFrontend.appProtocol.grpc }} | |
| {{- end }} |
metrics-generator:
helm-charts/charts/tempo-distributed/templates/metrics-generator/service-metrics-generator.yaml
Lines 24 to 26 in 76d7026
| {{- if and (hasPrefix .name "grpc") ($.Values.metricsGenerator.appProtocol.grpc) }} | |
| appProtocol: {{ $.Values.metricsGenerator.appProtocol.grpc }} | |
| {{- end }} |
|
@msgSend could you please also sign the DCO ? |
I could not get this to work correctly. I have them set, but I never see them rendered in the service manifest. |
1a3b9c6 to
6e6e5b6
Compare
6e6e5b6 to
e43409f
Compare
e43409f to
665f0bf
Compare
Signed-off-by: Erwin Tung <[email protected]>
Signed-off-by: Erwin <[email protected]> Signed-off-by: Sheikh-Abubaker <[email protected]>
1a3b9c6 to
e100b6b
Compare
@msgSend No worries! here's how you could utilize the existing appProtocol feature: In IMPORTANT NOTE: Set
|
| ipFamilyPolicy: {{ .Values.tempo.service.ipFamilyPolicy }} | ||
| ports: | ||
| - name: http-metrics | ||
| - appProtocol: grpc |
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 please help me understand the need of setting grpc protocol for a http-metrics port ?
Fixes #3281
Enabling streaming on Grafana does not work in certain cases (e.g. Istio) with the tempo-distributed Helm chart despite setting
stream_over_http_enabled: truein the config. This is because the appProtocol is not defined in the Kubernetes services and so the gRPC payload is munged.Relevant Istio reference: https://istio.io/latest/docs/ops/configuration/traffic-management/protocol-selection/#explicit-protocol-selection