helm-charts
helm-charts copied to clipboard
grafana
[tempo-distributed] Add OTEL gRPC support for the Tempo gateway
I'm trying to address this issue from a while back: https://github.com/grafana/helm-charts/issues/3218
Most of the credit to @Dipu18 who's had https://github.com/grafana/helm-charts/pull/3261 open for a while but never addressed @joe-elliott's comments. I took his NGINX config and wrapped it and the gRPC ports in a flag that's in the values, per Joe's request to do the ports in a cleaner way.
An alternative to having the flag be .Values.gateway.enableGrpc, we could also change it simply enable upon the global values that the distributor also uses to open up the port on its end, being .Values.traces.jaeger.grpc.enabled.
Let me know if there's anything else that needs to be done on my end.
This would be a great addition IMO. Not be pushy, but can we expect this to get reviewed/merged soon?
@jdegendt, @pkarakal apologies for the delay, currently there is a limited bandwidth from my end, planning to review it soon.
Thanks for the approval.
@Sheikh-Abubaker, could you have a look as well please so we can get this merged?
@Sheikh-Abubaker, could you have a look as well please so we can get this merged?
Hey @jdegendt, Thanks for your Contribution! just wondering if you had a chance to test this? could you please share a bit if this is working at your end as intended ?
@jdegendt Feel free to reach out in case of any doubts, or if you would like me to co author this PR. Thanks!
@Sheikh-Abubaker, thanks for the extensive review!
I've went ahead and removed the enableGrpc flag and used the existing otlp one instead. Additionally, the port's been made configurable as well.
Perhaps one question on how you'd like to approach this, the gateway service and NGINX config now use the configurable port from the values, with 4317 being the default. See here.
Note that in the proxy pass part of the NGINX config I do still use the standard otlp port when forwarding to the distributor, see.
Would you like me to extend this configurable port to the distributor service and deployment as well for their respective gRPC port, or rather not? (I personally wouldn't but figured I'd ask.)
Note that in the proxy pass part of the NGINX config I do still use the standard otlp port when forwarding to the distributor, see.
I tested it with the configurable value and it worked like charm, I believe that was probably overlooked from my side before and it would definitely be a good idea to replace the hardcoded port with the configurable value. I have addressed that in the above review as well
Would you like me to extend this configurable port to the distributor service and deployment as well for their respective gRPC port, or rather not? (I personally wouldn't but figured I'd ask.)
IMO, yes but I'd encourage a separate PR for it, leaving this one to gateway configurations only.
IMO, yes but I'd encourage a separate PR for it, leaving this one to gateway configurations only.
@Sheikh-Abubaker, alright, easy enough. I've got the MR for that ready here, I'll pull in main and bump the chart once this one's merged.
After making the required changes please add an explicit note in the
READMEon how to get started with this feature , the reason behind the ask is, this feature had been a popular topic among the community over the past couple of years and a note would hopefully help community to get started with it easily.
@jdegendt Everthing looks solid, you'd just need to run helm-docs from here, to update the docs, and could you please add an explicit note in the README.md as well, like mentioned above ?
@Sheikh-Abubaker, all done, let me know if my little explanation block suffices 👍
@Sheikh-Abubaker, all done, let me know if my little explanation block suffices 👍
@jdegendt Looks great! Thanks! since we're almost there I'll test it again and perhaps merge after the approval.
@Sheikh-Abubaker, all done, let me know if my little explanation block suffices 👍
Just tested it, Works like 🔥, Thank you so much for your patience and contribution! keep them coming!
Whoop! Thanks @Sheikh-Abubaker 🥳
This one's also ready to go now: https://github.com/grafana/helm-charts/pull/3794