[jaeger] Fix otlp service regression in 0.71.5

[spadger]

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 [email protected]

What this PR does

Fixes regression in 0.71.5

Which issue this PR fixes

Can't see one that has been raised

Checklist

• DCO signed
• Commits are GPG signed
• Chart Version bumped
• Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
• README.md has been updated to match version/contain new values

[mehta-ankit]

Suggested change

	name: otlp-grpc
	name: otlp-http

[spadger]

You're right that this was a mistake, but the actual format should have been http-otlp. Otherwise, we'd have caused a regression you attempted to fix in 0.71.5!

[mehta-ankit]

@spadger Thanks for catching this. It was my bad, I missed this in the previous MR.

[mehta-ankit]

We should also make deployment port names configurable.
https://github.com/cnvergence/jaeger-helm-charts/blob/main/charts/jaeger/templates/collector-deploy.yaml#L122

[spadger]

@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"

[spadger]

@mehta-ankit I can make them configurable, but could you help understand why we'd do this? As far as I understand, port names 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]

@mehta-ankit I can make them configurable, but could you help understand why we'd do this? As far as I understand, port names 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?

People had issues with certain port names because of Istio. I don't have all the details but this should explain: #344 (comment)

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 ?

[spadger]

OK, that's complete, and I found my own typo that caused the lining to fail

[mehta-ankit]

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]

@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:

• Most people won't care
• The istio crowd have get their fix for free

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)

[mehta-ankit]

@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:

• 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 two otlp-* 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.

[mehta-ankit]

@cnvergence FYI, #479 (comment)
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.

[mehta-ankit]

LGTM

[cnvergence]

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

[spadger]

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

[mehta-ankit]

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

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