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 Jaeger UI and service port according to docs for jaeger example #249

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

adilhafeez
Copy link
Contributor

@adilhafeez adilhafeez commented Oct 23, 2024

Default ports for jaeger and envoy service are incorrect in docker-compose file. In docs jager UI is set at 16686 and service's port is set to 8000

This code change updates ports according to docs.

Jaeger UI showing traces,
image

@adilhafeez adilhafeez marked this pull request as ready for review October 23, 2024 07:35
@adilhafeez adilhafeez changed the title Fix ports Update Jaeger UI and service port according to docs for jaeger example Oct 23, 2024
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

thanks for working on this @adilhafeez - comments below

jaeger-tracing/docker-compose.yaml Outdated Show resolved Hide resolved
@@ -40,4 +40,4 @@ services:
environment:
- COLLECTOR_ZIPKIN_HOST_PORT=9411
ports:
- "${PORT_UI:-10000}:16686"
- "${PORT_UI:-16686}:16686"
Copy link
Member

Choose a reason for hiding this comment

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

this is correct as its not being proxied so makes more sense to expose the default port in this case.

it would also ensure it matches what is written in the docs

@phlax
Copy link
Member

phlax commented Nov 6, 2024

@adilhafeez you will need to update the listener port in envoy.yaml also

@adilhafeez
Copy link
Contributor Author

@adilhafeez you will need to update the listener port in envoy.yaml also

@phlax just pushed another change. This change updates envoy with 10000 port and also updates rst file with console output.

adilhafeez and others added 4 commits November 6, 2024 09:14
Signed-off-by: Adil Hafeez <adil@katanemo.com>
Co-authored-by: phlax <phlax@users.noreply.github.com>
Signed-off-by: Adil Hafeez <adil@katanemo.com>
Signed-off-by: Adil Hafeez <adil@katanemo.com>
Signed-off-by: Adil Hafeez <adil@katanemo.com>
@phlax
Copy link
Member

phlax commented Nov 6, 2024

nice, thanks @adilhafeez

you will need to fix DCO now tho

@adilhafeez
Copy link
Contributor Author

@phlax do you think it's a bad idea to link the screenshot in rst file?

@phlax
Copy link
Member

phlax commented Nov 6, 2024

screenshot is fine but it will need to be added as an artefact - see the opentelemetry example for how to do that

Signed-off-by: Adil Hafeez <adil@katanemo.com>
Signed-off-by: Adil Hafeez <adil@katanemo.com>
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @adilhafeez - really nice

as an aside we need to get docs build previews working in this repo

@phlax phlax merged commit f44b535 into envoyproxy:main Nov 6, 2024
4 checks passed
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.

2 participants