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

Warn on a potentially incorrect HTTP OTLP endpoint #126

Merged
merged 10 commits into from
Sep 25, 2023

Conversation

keturiosakys
Copy link
Member

This is a small library quality of life PR that will warn a user of the HTTP OTLP exporter if they pass a URL to the init function that does not have the spec-compliant /v1/metrics endpoint.

@keturiosakys keturiosakys marked this pull request as ready for review September 15, 2023 13:54
@mellowagain
Copy link
Member

i think we should just add it automatically, thats the behaviour we have with the otel exporter in the rust library

@arendjr
Copy link
Collaborator

arendjr commented Sep 15, 2023

I actually don't think we should add anything to the path automagically, since there's nothing preventing people from using whatever custom paths they desire. So if we force-add the path, those people would have no way of configuring their custom URL.

Regardless, this might be something to properly specify in the Autometrics 1.0 spec, @hatchan

@gagbo
Copy link
Member

gagbo commented Sep 15, 2023

I think the spec forces this subpath, because the Go library does exactly this. You can configure the value for specific needs of course, but if you just pass the url like localhost:4318, the default configuration will add /v1/metrics to the path. Maybe that's the way to go? Default to adding the path, and for custom setups we provide a way to override the behaviour?

@hatchan
Copy link

hatchan commented Sep 19, 2023

I thought it was in the defaults that the OTEL collector has, but I'm not sure about the otel spec itself.

I find it pretty odd if a application or library does add something by itself, as @arendjr describes, there is no way of doing something else.

@keturiosakys
Copy link
Member Author

so one option we can take is where if the user submits just the base URL with the port http://otel-exporter:4317 we can assume they're using default Otel configurations that are expecting /v1/metrics endpoint - we can then automatically append that. If the URL specified has some other specific endpoint - we can just log a warning but leave it alone.

packages/exporter-otlp-http/src/index.ts Outdated Show resolved Hide resolved
);
}

if (urlObj.pathname === "/" && urlObj.port === "4317") {
Copy link
Collaborator

@arendjr arendjr Sep 25, 2023

Choose a reason for hiding this comment

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

I'm afraid there's still an edge case here. For instance:

new URL("http://localhost:4317").pathname === "/"

In other words, there will not be a distinction between users providing a URL with or without a path. Ie. "http://localhost:4317" and "http://localhost:4317/" are interchangeable. But I don't think that's what we want if we automatically add the default path, since the URL ending with / contains an explicit path and should not be modified, or otherwise we prevent people from submitting to the root path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good catch. I'll add an exception for cases when root path / is explicitly referred to.

@keturiosakys
Copy link
Member Author

[TODO]: figure out a test for this 🙂

@keturiosakys keturiosakys merged commit 57bc121 into main Sep 25, 2023
1 check passed
@keturiosakys keturiosakys deleted the add-endpoint-automatically branch September 25, 2023 13:00
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.

5 participants