Skip to content
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

Update sidecar example in Operator README #183

Merged

Conversation

erichsueh3
Copy link
Contributor

@erichsueh3 erichsueh3 commented Apr 25, 2022

This PR updates the sidecar example in the Operator README, as it does not work. It changes the jaegerreceiver protocol to thrift_compact instead of grpc. This is also reflected in a PR on the opentelemetry-operator repository, 837.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make the example correct by making it be OTLP instead of jaeger.

otlp:
  protocols:
    grpc:
    http:
...
service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [logging]

I suggest doing the same in the operator repo's README

@TylerHelmuth
Copy link
Member

@erichsueh3 please bump the operator chart version.

@erichsueh3
Copy link
Contributor Author

Let's make the example correct by making it be OTLP instead of jaeger.

otlp:
  protocols:
    grpc:
    http:
...
service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [logging]

I suggest doing the same in the operator repo's README

@TylerHelmuth The example does not work with the otlp receiver, I think because the myapp pod uses image: jaegertracing/vertx-create-span:operator-e2e-tests.

@TylerHelmuth
Copy link
Member

The example does not work with the otlp receiver, I think because the myapp pod uses image: jaegertracing/vertx-create-span:operator-e2e-tests.

I should've read down further to see the image. Thanks for the clarification.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why grpc isn't working? It is the default protocol according to the receiver.

@TylerHelmuth TylerHelmuth self-requested a review April 25, 2022 22:21
@erichsueh3
Copy link
Contributor Author

Do you know why grpc isn't working? It is the default protocol according to the receiver.

What do you mean by default protocol? A protocol must be specified in the jaegerreceiver configuration, and in the Getting Started section of that README it says "By default, the Jaeger receiver will not serve any protocol".

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Apr 25, 2022

What do you mean by default protocol? A protocol must be specified in the jaegerreceiver configuration, and in the Getting Started section of that README it says "By default, the Jaeger receiver will not serve any protocol".

You're right, shouldve said "a protocol". I would expect grpc as a protocol to work, unless the jaegertracing/vertx-create-span:operator-e2e-tests image doesn't know how to send over grpc. My guess is it only knows how to send over thrift_compact?

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming jaegertracing/vertx-create-span:operator-e2e-tests can't send over GRPC and can send over thrift_compact then this looks good to me. I would hold off on merging until the operator PR is approved, to ensure this example stays up-to-date with any feedback from that PR.

@erichsueh3
Copy link
Contributor Author

Assuming jaegertracing/vertx-create-span:operator-e2e-tests can't send over GRPC and can send over thrift_compact then this looks good to me. I would hold off on merging until the operator PR is approved, to ensure this example stays up-to-date with any feedback from that PR.

Yes, after testing it myself, it seems like it could only send over thrift_compact so it's most likely that jaegertracing/vertx-create-span:operator-e2e-tests can't send over GRPC.

@dmitryax dmitryax merged commit 964fa85 into open-telemetry:main Apr 26, 2022
@erichsueh3 erichsueh3 deleted the operator_README_sidecar_update branch April 26, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants