-
Notifications
You must be signed in to change notification settings - Fork 9
feat(cat-gateway): Enable telemetry #3838
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
base: main
Are you sure you want to change the base?
Conversation
📚 Docs PreviewThe docs for this PR can be previewed at the following URL: |
| // Returns optional guard for telemetry providers. | ||
| // We must hold this guard while the main process runs. | ||
| // No need to do anything else. | ||
| let _guard = Settings::init(settings)?; |
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.
What do you think to make this telemetry guard as s static variable and manage it just inside the telemetry module ?
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.
We could make it a static variable, but we would need to call the shutdown functions in the OTEL providers before the main program exits explicitly. In my consideration, there is no need to store this as a static variable (i.e. there is no need to reference the guard at any other point during the execution of the program).
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.
Nitpicking.
What do you think just extend the original catalyst-gateway/README.md with how to run telemetry stuff and extend the catalyst-gateway/docker-compose.yml file with the necessary "telemetry" services ?
The main reason is to keep everything in one place, so it would be easier to maintain
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.
This can be done, I just wanted to keep things separate such that review of the telemetry stuff can be done cleanly. We still have to cleanup the docker-compose in the demo/ path, so that the prometheus container can scrape from the cat-gateway and jaeger containers.
c3a3b31 to
a9c834c
Compare
Description
Adds software dependencies to enable opentelemetry traces.
Closes #3682 .
Description of Changes
Provides a demonstration of how to enable opentelemetry traces in the existing
cat-gateway.Please confirm the following checks