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

docker: add pulsar example #3789

Merged

Conversation

CodePrometheus
Copy link
Contributor

@CodePrometheus CodePrometheus commented Jan 30, 2025

Since there is no collector-pulsar module at present, compiling docker will result in an error, so when this PR is merged, I will add zipkin-collector-pulsar to the github actions in #3788.

@CodePrometheus
Copy link
Contributor Author

@codefromthecrypt Could you kindly review this when you have some time? Thank you!

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

I think for this change, in order to be a proper dependency, should remove the docker-compose examples until there's server code to use them. Otherwise, I think it looks good to me as this should be enough to publish the image!

container_name: pulsar
ports: # expose the pulsar port so apps can publish spans.
- "6650:6650"
- "8080:8080"
Copy link
Member

Choose a reason for hiding this comment

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

when ready for this change (docker-compose), note this is a common port, so maybe comment it out and say "uncomment to expose the UI port"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll modify it when it's ready.

# Usually, we read env set from pid 1 to get docker-healthcheck parameters.
# However, pulsar-server has to start as root even if permissions are dropped
# later. So, we expose it in the Dockerfile instead.
ENV HEALTHCHECK_PORT=8080
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we check the service port 6650 (esp if we are using tcp)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified it to HTTP on port 8080, which is enough for the healthcheck.

Copy link
Member

Choose a reason for hiding this comment

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

ok mainly I was concerned about race condition where 8080 is ready, but service port not. If this becomes a problem we can revisit

@codefromthecrypt
Copy link
Member

looks like the healthcheck approach is failing on 404. you will need a valid http path

https://github.com/openzipkin/zipkin/actions/runs/13084307860/job/36514939047?pr=3789

also, once done please lower the log level to warn as it will be unreadable to have info due to the health checks (examples shouldn't scroll too long due to chatty or health check in docker, we usually set to WARN)

@CodePrometheus
Copy link
Contributor Author

Well, I have tested it locally and it should be OK now.

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

Small charge then merge. Thanks!

docker/examples/README.md Outdated Show resolved Hide resolved
@CodePrometheus
Copy link
Contributor Author

It seems that the error is unrelated to the changes and may be due to a network issue, could you please run it again.

@codefromthecrypt codefromthecrypt merged commit 43f88ea into openzipkin:master Feb 3, 2025
14 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.

3 participants