Remove explicit base path setting#7682
Remove explicit base path setting#7682ycombinator wants to merge 3 commits intoelastic:masterfrom ycombinator:metricbeat/host-parser/support-basepaths
Conversation
|
/cc @chrisronline as an FYI, since you might be testing metricbeat+kibana with base paths. |
|
Legitimate test failure: I'm looking into it... |
| {"", "", "empty host"}, | ||
| {":80", "", "empty host"}, | ||
| {"localhost", "http://localhost/server-status?auto=", ""}, | ||
| {"localhost/ServerStatus", "http://localhost/ServerStatus?auto=", ""}, |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
LGTM.
@andrewkroh Could you have a look?
andrewkroh
left a comment
There was a problem hiding this comment.
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.
|
Sounds good, @andrewkroh. Thanks! For clarity I'm going to close this PR and create a new one that implements this:
|
|
Replaced by #7700. |
In #7525 I introduced an explicit setting allowing modules using the
URLHostParserBuilderto 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
hostssetting. This PR makes this change in the implementation.So, instead of having to do something like this:
Users could simply do:
Specifically this PR:
URLHostParserBuilderto remove support for an explicit base path setting,kibanamodule code, andURLHostParseBuildercode to use a base path implicitly specified as a suffix in thehostssetting, for example:hosts: ["localhost:5601/foo"]