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

[FrontendProxy] Add tracing to Envoy #613

Merged

Conversation

julianocosta89
Copy link
Member

@julianocosta89 julianocosta89 commented Nov 30, 2022

Changes

As Envoy supports OpenTelemetry, I believe it would be great if we could use it.
https://www.envoyproxy.io/docs/envoy/latest/start/sandboxes/opentelemetry

This PR adds the configuration required to add tracing to Envoy.

jaeger-proxy

Merge Requirements

For new features contributions please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs folder

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@julianocosta89 julianocosta89 requested a review from a team November 30, 2022 11:48
@julianocosta89 julianocosta89 added the enhancement New feature or request label Nov 30, 2022
@julianocosta89 julianocosta89 self-assigned this Nov 30, 2022
@cartersocha
Copy link
Contributor

We should add envoy to the current services architecture doc. Especially if there's a span coming from it

Should we add a simple envoy service doc with config instructions too?

@julianocosta89
Copy link
Member Author

@cartersocha Not sure what exactly to write, but I've added the "how to" make Envoy produce spans.

@cartersocha
Copy link
Contributor

I'm seeing errors on the frontend and checkout service

@cartersocha
Copy link
Contributor

Not sure why the shipping service is throwing issues.

"13 INTERNAL: shipping quote failure: failed to get shipping quote: rpc error: code = Unknown desc = error sending request for url (http://quoteservice:8090/getquote): error trying to connect: dns error: failed to lookup address information: Name does not resolve"

@cartersocha
Copy link
Contributor

A system prune cleared up the errors but I'm not seeing an envoy span in any trace. I see envoy as an orphan span currently and the main trace doesn't have it present

@julianocosta89
Copy link
Member Author

@cartersocha true!
Something that I forgot to mention is that this is generating spans only when you navigate to the OTel-Demo page.
The loadgen doesn't send requests through the envoy, but to the frontend container directly.

@cartersocha
Copy link
Contributor

I see! lgtm.

This is where it would be nice to have non synthetic requests set the attribute to filter on.

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

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

Please add a note in the envoy services doc at the top saying only non synthetic requests will trigger the envoy tracing.

Lgtm otherwise

Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

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

One small nit. everything works as expected. 👍

docker-compose.yml Show resolved Hide resolved
@cartersocha cartersocha added the in-progress This issue is actively being worked on label Dec 7, 2022
@puckpuck puckpuck merged commit 5c06d52 into open-telemetry:main Dec 7, 2022
@julianocosta89 julianocosta89 deleted the frontendproxy-add-envoy-tracing branch December 7, 2022 13:57
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* Add tracing to Envoy

* Changelog

* yamllint

* lint

* Add doc

* Add note to doc

* Move env var definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in-progress This issue is actively being worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants