-
Notifications
You must be signed in to change notification settings - Fork 450
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
Add appProtocol for otlp and jaeger receiver parsers #704
Conversation
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.
LGTM,
if you could fix as well Zipkin that would be great, it's often times used in service mesh.
It would be great as well improve e2e test and assert the service e.g. https://github.com/open-telemetry/opentelemetry-operator/blob/main/tests/e2e/smoke-simplest/00-assert.yaml
Few tests are failing in the CI more details: https://github.com/open-telemetry/opentelemetry-operator/runs/5137930906?check_suite_focus=true#step:7:776 |
94d1c97
to
17db6e6
Compare
yeah, it's quite tricky to run the full suite on mac, but it should be fine now. |
for the zipkin part, I'm not really familiar with it and looks like it use GenericReceiver, so it's possible to setup TCP if the name is zipkin, but I'm not sure about appProtocol, it should be http, http2 or grcp? Looks like it depends https://zipkin.io/pages/tracers_instrumentation |
Zipkin uses http |
17db6e6
to
97d4221
Compare
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.
Please add the e2e test- verify the service has the protocol set.
@@ -53,6 +53,16 @@ func (g *GenericReceiver) Ports() ([]corev1.ServicePort, error) { | |||
} | |||
|
|||
if g.defaultPort > 0 { | |||
if g.name == "zipkin" { |
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.
I am not sure about this approach. The protocol type should be rather set in
return &GenericReceiver{ |
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.
@pavolloffay just to be clear for zipkin config we can expect something like this:
zipkin:
---
zipkin:
protocols:
http:
endpoint: "0.0.0.0:9999"
And in case of default config "zipkin:" is this ok to add to GenericReceiver fields like defaultProtocol and defaultAppProtocol?
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.
I think we will have to add a field there
Signed-off-by: Sergei Semenchuk <[email protected]>
97d4221
to
6026da6
Compare
) Signed-off-by: Sergei Semenchuk <[email protected]>
Signed-off-by: Sergei Semenchuk [email protected]