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 OTel for better tracing #41

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Update OTel for better tracing #41

merged 3 commits into from
Oct 2, 2024

Conversation

NeuralFlux
Copy link
Contributor

linked to biothings/biothings_explorer#857

Note

requires new environment variable OTEL_EXPORTER_OTLP_ENDPOINT

@tokebe
Copy link
Member

tokebe commented Sep 17, 2024

Is it at all possible to not require a new environment variable through exporter config? A new environment variable would require changes with ITRB.

@NeuralFlux
Copy link
Contributor Author

NeuralFlux commented Sep 17, 2024

@tokebe I didn't know that. I created a new var because I did not find any reference to the old one jaeger-otel-agent.sri. Main question is: does that var include the protocol or only has the hostname?

@tokebe
Copy link
Member

tokebe commented Sep 17, 2024

The old variables were JAEGER_HOST and JAEGER_PORT, containing only the hostname and port, respectively. IIRC we don't actually rely on the environment variables and just fall back to the default, but want the ability to set them if they change in the future.

It looks like you can configure the exporter instance where it takes a url which is the combined host:port/endpoint. You should be fine just using string formatting and the existing variables/their defaults to create that.

@NeuralFlux
Copy link
Contributor Author

The new exporter uses HTTP over UDP, which was used by the old exporter. So is it okay to change the default port and endpoint to 4318 and /v1/traces respectively? Jaeger ref

@tokebe
Copy link
Member

tokebe commented Sep 17, 2024

Makes sense to me. I'll double check with the team in charge of the production jaeger instance.

@tokebe tokebe merged commit c1146be into main Oct 2, 2024
2 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