Closed spadger closed 1 year ago
@spadger Thanks for catching this. It was my bad, I missed this in the previous MR.
@spadger Thanks for catching this. It was my bad, I missed this in the previous MR.
Welcome. I just saw the linter output - looks like I might not have done any better
Error: Service "jaeger-flmntaf7jw-collector" is invalid: spec.ports[3].name: Duplicate value: "otlp-grpc"
@mehta-ankit I can make them configurable, but could you help understand why we'd do this? As far as I understand, port name
s are just arbitrary tokens that k8s uses to match a service's port with a pod's port. A hard-coded otlp-grpc
is descriptive, so could you explain the case where someone would change this?
@mehta-ankit I can make them configurable, but could you help understand why we'd do this? As far as I understand, port
name
s are just arbitrary tokens that k8s uses to match a service's port with a pod's port. A hard-codedotlp-grpc
is descriptive, so could you explain the case where someone would change this?
People had issues with certain port names because of Istio. I don't have all the details but this should explain: https://github.com/jaegertracing/helm-charts/issues/344#issuecomment-1583708032
I agree with you, but keeping them configurable and making the OG values the default, its a win-win for us who dnt need to change port names and for others who are having issues. Do you agree ?
OK, that's complete, and I found my own typo that caused the lining to fail
OK, that's complete, and I found my own typo that caused the lining to fail
@spadger i am confused. Why are we not changing the values.yaml to be what it was before 0.71.5 change:
otlp-grpc
& otlp-http
??
Look at this commit: https://github.com/jaegertracing/helm-charts/pull/478/files#diff-30fbc05152b26afaa2337764ff007b0240c79e25d76fb7ec985a18adc0ee3140
@spadger i am confused. Why are we not changing the values.yaml to be what it was before 0.71.5 change:
otlp-grpc
&otlp-http
?? Look at this commit: https://github.com/jaegertracing/helm-charts/pull/478/files#diff-30fbc05152b26afaa2337764ff007b0240c79e25d76fb7ec985a18adc0ee3140
The istio bug you pointed at was caused because the port name needed to start with http-
, and I saw here that you even started to make that change yourself to values.yaml
. I figured it'd be better to keep the port names named like that by default because:
If you prefer, I'm happy to change the port names to otlp-grpc
& otlp-http
, and let the istio folks override config (I'm not fussed either way, because two otlp-*
together is more satisfying to read :p)
@spadger i am confused. Why are we not changing the values.yaml to be what it was before 0.71.5 change:
otlp-grpc
&otlp-http
?? Look at this commit: https://github.com/jaegertracing/helm-charts/pull/478/files#diff-30fbc05152b26afaa2337764ff007b0240c79e25d76fb7ec985a18adc0ee3140The istio bug you pointed at was caused because the port name needed to start with
http-
, and I saw here that you even started to make that change yourself tovalues.yaml
. I figured it'd be better to keep the port names named like that by default because:
- Most people won't care
- The istio crowd have a workaround
If you prefer, I'm happy to change the port names to
otlp-grpc
&otlp-http
, and let the istio folks override config (I'm not fussed either way, because twootlp-*
together is more satisfying to read :p)
Yea i feel, we should keep these changes backwards compatible. I made the same comment in previous MR (0.71.5) but missed the values change.
The whole point of making port names configurable is that users can make it whatever they want, but we do not change the behavior for anyone using old names that were set in the values file.
@cnvergence FYI, https://github.com/jaegertracing/helm-charts/pull/479#issuecomment-1604489712 Since you made the previous change, I wanted to let you know that you would want to change values for your helm chart deployment to match whatever you need. In this PR we are making those values what they were before your change so as to not break backwards compatibility.
Oh, nice catch, thanks @spadger @mehta-ankit and I fully agree that the default names should have old values. I completely missed that when I was refactoring my PR, sorry about that
Hey @mehta-ankit
So I happen to be using jaeger in an istio environment. Nothing as special like the bug you linked to; simply "We have an isito gateway, and want to report otlp spans". Because the port doesn't start with grpc-
, Istio sends the spans as an http POST instead of a gRPC call. Do you reckon its worth switching the ports to grpc-otlp
and http-otlp
to fix this papercut? I was really lucky because after a few hours of head-banging, I remembered the port-naming chart feature, but others may not be so lucky
Hey @mehta-ankit So I happen to be using jaeger in an istio environment. Nothing as special like the bug you linked to; simply "We have an isito gateway, and want to report otlp spans". Because the port doesn't start with
grpc-
, Istio sends the spans as an http POST instead of a gRPC call. Do you reckon its worth switching the ports togrpc-otlp
andhttp-otlp
to fix this papercut? I was really lucky because after a few hours of head-banging, I remembered the port-naming chart feature, but others may not be so lucky
Hi @spadger I don't think using Istio is such a common thing where we should change default values which could affect anyone else depending on older names for whatever reason. Do you think its a better idea maybe to add some docs around Istio and port name oddity ? https://github.com/jaegertracing/helm-charts/tree/main/charts/jaeger
Version 0.71.5 added configurable otlp port names to the collector service, but introduced some typos, which break otlp for http & gRPC. I'm not sure why they would be configurable, so I haven't also made the port names on the actual deployment configurable to match the service, but at least this should fix connectivity Signed-off-by: Jon Bates jonathan.bates@team.bumble.com
What this PR does
Fixes regression in 0.71.5
Which issue this PR fixes
Can't see one that has been raised
Checklist
[jaeger]
or[jaeger-operator]
)