Remove explicit base path setting

[ycombinator]

In #7525 I introduced an explicit setting allowing modules using the URLHostParserBuilder to specify a base path. The idea was that this base path would be inserted in URLs after the host:port but before the actual resource path.

The motivation behind such a setting was to account for HTTP applications that are sometimes hosted behind a proxy along side other HTTP applications and, as such, use a base path at the proxy layer to differentiate requests between applications. Kibana, for instance, supports this use case and allows users to configure a base path to be used in all its URLs.

While the motivation is valid, my implementation in #7525 (by providing an explicit setting for base paths) was unnecessarily complex. As @andrewkroh suggested, it would be better for users if they could simply specify the base path as part of the hosts setting. This PR makes this change in the implementation.

So, instead of having to do something like this:

hosts: ["localhost:5601"]
basepath: "foo"

Users could simply do:

hosts: ["localhost:5601/foo"]

Specifically this PR:

• Updates the code for URLHostParserBuilder to remove support for an explicit base path setting,
• Removes usage of explicit base path setting from kibana module code, and
• Instead makes it possible for URLHostParseBuilder code to use a base path implicitly specified as a suffix in the hosts setting, for example: hosts: ["localhost:5601/foo"]

[ycombinator]

/cc @chrisronline as an FYI, since you might be testing metricbeat+kibana with base paths.

[ycombinator]

Legitimate test failure:

09:44:44 --- FAIL: TestHostParser (0.00s)
09:44:44 	assertions.go:247: 
09:44:44                           
09:44:44 	Error Trace:	status_test.go:240
09:44:44 		
09:44:44 	Error:      	Not equal: 
09:44:44 		
09:44:44 	            	expected: "http://localhost/ServerStatus?auto="
09:44:44 		
09:44:44 	            	actual  : "http://localhost/ServerStatus/server-status?auto="
09:44:44 		
09:44:44 	Test:       	TestHostParser
09:44:44 FAIL

I'm looking into it...

[ycombinator]

This test was failing due to the changes in this PR. I decided to remove it because the intent behind this test seems to be to override the apache server status path, which can be accomplished via the server_status_path setting in the apache module (which defaults to "server-status").

[ruflin]

LGTM.

@andrewkroh Could you have a look?

[andrewkroh]

I like the changes you did here, but I'm pretty sure that this would introduce a breaking change that would affect existing configurations (outside of the kibana module) so we cannot proceed.

Users that have overridden the default path by including a path in the hosts URL would find their configurations are broken as exemplified by the failing apache status test. Several other metricset like dropwizard could be affected too depending on the user config.

hosts: [http://jvm:18080/myapp/dropwizard/mymetrics]

• Current: /myapp/dropwizard/mymetrics
• With this change: /myapp/dropwizard/mymetrics/metrics/metrics

# Workaround
hosts: [http://jvm:18080/myapp/dropwizard/mymetrics]
path: ""

So I think you arrived at the best possible solution with basepath (and sorry for steering you in the wrong direction). In order to make the settings consistent across metricsets, I suggest removing BasePathConfigKey and rely on it always being basepath.

[ycombinator]

Sounds good, @andrewkroh. Thanks! For clarity I'm going to close this PR and create a new one that implements this:

In order to make the settings consistent across metricsets, I suggest removing BasePathConfigKey and rely on it always being basepath.

[ycombinator]

Replaced by #7700.