Explicitly set Service Port Protocol for Jaeger Receivers #117
Conversation
This file contains 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
|
I have now signed the CLA, but I don't know why the lint timed out and failed unfortunately. |
|
Thanks for the PR! I'm unable to review it today, but I'll take a look soon. |
jpkrohling
reviewed
Nov 23, 2020
The servicePort created for each Jaeger receiver does not currently set a Protocol, and therefore defaults to TCP. Some Jaeger receiver protocols (i.e. thrift_binary and thrift_compact) are UDP based rather than TCP, and therefore the operator currently creates service port entries that cannot be used due to the protocol mismatch.
Reduces code duplication by setting the port's protocol consistently, rather than conditionally based the supplied configuration.
|
Thanks for the review! I've resolved the issue you found and re-based with the latest changes. I've re-run the tests locally and re-confirmed that it also works as expected on my cluster. |
jpkrohling
reviewed
Nov 30, 2020
Rename protocol to transportProtocol, where protocol refers to a transport protocol (i.e. TCP/UDP) to reduce ambiguity between transport protocols and application protocols.
jpkrohling
approved these changes
Dec 1, 2020
Awesome! Thank you for your review and constructive feedback too! |
shree007
pushed a commit
to shree007/opentelemetry-operator
that referenced
this pull request
Dec 12, 2021
…etry#117) The service port created for each Jaeger receiver does not currently set a Protocol, and therefore defaults to TCP. Some Jaeger receiver protocols (i.e. thrift_binary and thrift_compact) are UDP based rather than TCP, and therefore the operator currently creates service port entries that cannot be used due to the protocol mismatch. For example, the following manifest will currently create service port entries with the wrong protocol for thrift_compact and thrift_binary protocols; ``` apiVersion: opentelemetry.io/v1alpha1 kind: OpenTelemetryCollector metadata: name: simplest spec: config: | receivers: jaeger: protocols: grpc: thrift_compact: thrift_binary: thrift_http: processors: queued_retry: exporters: logging: loglevel: debug service: pipelines: traces: receivers: [jaeger] processors: [queued_retry] exporters: [logging] ``` ``` Type: ClusterIP IP: 10.233.59.118 Port: jaeger-grpc 14250/TCP TargetPort: 14250/TCP Endpoints: 10.233.110.188:14250 Port: jaeger-thrift-http 14268/TCP TargetPort: 14268/TCP Endpoints: 10.233.110.188:14268 Port: jaeger-thrift-compact 6831/TCP TargetPort: 6831/TCP Endpoints: 10.233.110.188:6831 Port: jaeger-thrift-binary 6832/TCP TargetPort: 6832/TCP Endpoints: 10.233.110.188:6832 ``` This PR fixes this behavior by extending the protocol struct to also include each Jaeger receiver protocol's protocol and set the service port's protocol when a new Jaeger receiver is created. I have also updated the Jaeger receiver test cases to expect the correct service port protocol for each Jaeger receiver protocol, and they all pass. I have also tested this manually against my own cluster and confirmed that the service port protocol is set for UDP for thrift_binary and thrift_compact both when an endpoint is explicitly defined and when left unset. I'm still quite new to OpenTelemetry, so i'm open to feedback if i've misunderstood things!
ItielOlenick
pushed a commit
to ItielOlenick/opentelemetry-operator
that referenced
this pull request
May 1, 2024
…etry#117) The service port created for each Jaeger receiver does not currently set a Protocol, and therefore defaults to TCP. Some Jaeger receiver protocols (i.e. thrift_binary and thrift_compact) are UDP based rather than TCP, and therefore the operator currently creates service port entries that cannot be used due to the protocol mismatch. For example, the following manifest will currently create service port entries with the wrong protocol for thrift_compact and thrift_binary protocols; ``` apiVersion: opentelemetry.io/v1alpha1 kind: OpenTelemetryCollector metadata: name: simplest spec: config: | receivers: jaeger: protocols: grpc: thrift_compact: thrift_binary: thrift_http: processors: queued_retry: exporters: logging: loglevel: debug service: pipelines: traces: receivers: [jaeger] processors: [queued_retry] exporters: [logging] ``` ``` Type: ClusterIP IP: 10.233.59.118 Port: jaeger-grpc 14250/TCP TargetPort: 14250/TCP Endpoints: 10.233.110.188:14250 Port: jaeger-thrift-http 14268/TCP TargetPort: 14268/TCP Endpoints: 10.233.110.188:14268 Port: jaeger-thrift-compact 6831/TCP TargetPort: 6831/TCP Endpoints: 10.233.110.188:6831 Port: jaeger-thrift-binary 6832/TCP TargetPort: 6832/TCP Endpoints: 10.233.110.188:6832 ``` This PR fixes this behavior by extending the protocol struct to also include each Jaeger receiver protocol's protocol and set the service port's protocol when a new Jaeger receiver is created. I have also updated the Jaeger receiver test cases to expect the correct service port protocol for each Jaeger receiver protocol, and they all pass. I have also tested this manually against my own cluster and confirmed that the service port protocol is set for UDP for thrift_binary and thrift_compact both when an endpoint is explicitly defined and when left unset. I'm still quite new to OpenTelemetry, so i'm open to feedback if i've misunderstood things!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The service port created for each Jaeger receiver does not currently set a Protocol, and therefore defaults to TCP. Some Jaeger receiver protocols (i.e. thrift_binary and thrift_compact) are UDP based rather than TCP, and therefore the operator currently creates service port entries that cannot be used due to the protocol mismatch. For example, the following manifest will currently create service port entries with the wrong protocol for thrift_compact and thrift_binary protocols;
This PR fixes this behavior by extending the protocol struct to also include each Jaeger receiver protocol's protocol and set the service port's protocol when a new Jaeger receiver is created. I have also updated the Jaeger receiver test cases to expect the correct service port protocol for each Jaeger receiver protocol, and they all pass. I have also tested this manually against my own cluster and confirmed that the service port protocol is set for UDP for thrift_binary and thrift_compact both when an endpoint is explicitly defined and when left unset.
I'm still quite new to OpenTelemetry, so i'm open to feedback if i've misunderstood things!