-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
docs: Add HTTP port to collector docker command #5441
Conversation
The docker run command includes a port for GRPC. If someone is using an HTTP OTLP exporter along with the collector, this command will not work out of the box. This commit adds the HTTP port to the docker run command.
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. @open-telemetry/collector-approvers, PTAL. Thanks.
@@ -61,7 +61,8 @@ preferred shell. | |||
|
|||
```sh | |||
docker run \ | |||
-p 127.0.0.1:4317:4317 \ | |||
-p 127.0.0.1:4317:4317 \ # for gRPC |
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 the comment here will cause problems
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.
$ docker run \
-p 127.0.0.1:4317:4317 \ # for gRPC
-p 127.0.0.1:4318:4318 \ # for HTTP
-p 127.0.0.1:55679:55679 \
otel/opentelemetry-collector-contrib
docker: invalid reference format.
See 'docker run --help'.
zsh: command not found: -p
zsh: command not found: -p
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.
tried in bash as well to make sure
bash-3.2$ docker run \ -p 127.0.0.1:4317:4317 \ # for gRPC -p 127.0.0.1:4318:4318 \ # for HTTP -p 127.0.0.1:55679:55679 \ otel/opentelemetry-collector-contrib
docker: invalid reference format.
See 'docker run --help'.
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.
Thanks for this addition @kaylareopelle.
@codeboten, right, I'm not surprised that we can't have comments after end of line continuation characters.
All: this being a quick start, I'd suggest just dropping the port-use comments.
Or even just above the docker run command:
|
Great point, @codeboten! My bad for not testing the snippet with the comments before opening this PR. I updated the docs to place the comment at the top in 0b0a0d4. However, I'm also open to removing it entirely. Please take another look when you have a chance! |
@@ -60,8 +60,10 @@ preferred shell. | |||
3. Launch the Collector: | |||
|
|||
```sh | |||
# 4317 for gRPC, 4318 for HTTP, 55679 for ZPages |
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 still feel it better to just drop the comment, but will let other @open-telemetry/docs-approvers chime in. If we keep it, let's be clear that this comment is referring to port numbers.
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.
Can we move that comment into the text above, like "Launch the Collector, listening on ports 4317 (for OTLP grPC), 4318 (for OTLP HTTP) and 55679 (for ZPages):"
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 don't really understand what the |
/fix:format |
You triggered fix:format action run at https://github.com/open-telemetry/opentelemetry.io/actions/runs/11694711177 |
IMPORTANT: (RE-)RUN
|
Hi @kaylareopelle. The file formatting check flagged the line length: our linter looks for 80 characters per line. I've fixed it with a GitHub action. You should be all set now. |
@chalin - Could you take another look? Since you requested changes, I need your approval before we can move forward. |
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.
Thanks for the ping.
LGTM ✨
Co-authored-by: opentelemetrybot <[email protected]> Co-authored-by: Tiffany Hrabusa <[email protected]>
The docker run command includes a port for GRPC. If someone is using an HTTP OTLP exporter along with the collector, this command will not work out of the box.
This commit adds the HTTP port to the docker run command.
Discovered by @wsmoak in a discussion on: open-telemetry/opentelemetry-ruby#1525 (comment)