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

Modifying how we handle OTEL traces/metrics endpoints #300

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

mariomac
Copy link
Contributor

@mariomac mariomac commented Sep 20, 2023

Investigating about #297, I realized an edge case in which a user setting the endpoint value via the endpoint YAML property would get automatically attached the /v1/metrics or /v1/traces path.

Since there each endpoint sets the value separately for their respective metrics or traces exporter, I think they should behave like the OTEL_EXPORTER_OTLP_TRACES_ENDPOINT or OTEL_EXPORTER_OTLP_METRICS_ENDPOINT env vars (use the endpoint without modification) instead of the current OTEL_EXPORTER_OTLP_ENDPOINT (add /v1/metrics or /v1/traces accordingly).

There was also the issue that we indiscriminately added the /v1/metrics or /v1/traces path suffix even if a user explicitly set the values through the OTEL_EXPORTER_OTLP_TRACES_ENDPOINT or OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. That means that an OTEL ingestor not exposing these standard paths but custom ones would not properly work with Beyla.

The breaking change relies in the way the YAML file is handled, and that we now stick to the standard.

This PR also refines the way the different exporter options are tested.

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM! Nice catch!

@mariomac mariomac merged commit 6691e9b into grafana:main Sep 21, 2023
4 checks passed
@mariomac mariomac deleted the endpoint-handling branch September 21, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants