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

Feature/read metrics from unix socket npipe #14558

Merged
merged 14 commits into from
Nov 22, 2019

Conversation

ph
Copy link
Contributor

@ph ph commented Nov 15, 2019

Added more logic inside the metricbeat url parser to differentiate the "http+unix" and the "http+npipe" case. This should not have any impact on any current module and allow to "add" the behavior to the helper.NewHTTP without introducing any breaking changes.

@ph ph added the in progress Pull request is currently in progress. label Nov 15, 2019
metricbeat/mb/parse/url.go Outdated Show resolved Hide resolved
@ph
Copy link
Contributor Author

ph commented Nov 18, 2019

jenkins test this please.

@ph ph marked this pull request as ready for review November 18, 2019 20:23
@ph ph changed the title [WiP] Feature/read metrics from unix socket npipe Feature/read metrics from unix socket npipe Nov 18, 2019
@ph ph requested a review from ycombinator November 18, 2019 20:23
@ph ph added review and removed in progress Pull request is currently in progress. labels Nov 18, 2019
@ph ph added in progress Pull request is currently in progress. [zube]: In Progress and removed Team:Beats review labels Nov 19, 2019
@ph
Copy link
Contributor Author

ph commented Nov 20, 2019

Had a good chat with @ycombinator yesterday and after going through the hell together we agreed the easiest solution is to actually used a combined scheme, in that case unix+http:// to correctly differentiate the "protocol" (http) from the "transport" (unix). To do this we will add new fields in the HostData that will encapsulate the kind of transport and the transport part (the unix domain socket)) and the NewHTTP function will take it into consideration to correctly generate the appropriate dialer.

@ycombinator
Copy link
Contributor

ycombinator commented Nov 22, 2019

Thanks @ph. This is definitely looking nicer!

After looking at the switch statement in makeDialier, I wonder if it might cleaner to define a TransportDialerMaker interface with a makeDialer() method on it. Then instead of the various Transports being represented as enumerations, they could be actual structs that implement this interface. That way each transport will know how to setup its own dialer, which seems cleaner to me. It will also let us encapsulate the TransportPath field in only those transports that need it, rather than "leaking" it into mb.HostData.

WDYT?

@ph
Copy link
Contributor Author

ph commented Nov 22, 2019

I think the method vs interface a bit equivalent in such a small set of code. But if desired I can make the change.

@ycombinator
Copy link
Contributor

I'm not married to having the interface. The main point was more about having structs for different transport types and each of them implementing their own MakeDialer method, plus the unix and npipe structs encapsulating the transport path within them.

@ph
Copy link
Contributor Author

ph commented Nov 22, 2019

Okay, I've made the change, I just need to fix a cyclic import and things should be fine.

@ph
Copy link
Contributor Author

ph commented Nov 22, 2019

@ycombinator ready for review, had to recheck everything on windows to make sure I didn't broke anything with files with custom build tags.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Left one nitpick comment. Overall LGTM!

@ph ph merged commit baf8c1d into elastic:master Nov 22, 2019
@ph ph added the needs_backport PR is waiting to be backported to other branches. label Nov 22, 2019
ph added a commit to ph/beats that referenced this pull request Nov 22, 2019
Allow monitoring information from libbeat to be retrieved over unix domain socket and named pipes.

(cherry picked from commit baf8c1d)
@ph ph added v7.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Nov 22, 2019
ph added a commit that referenced this pull request Nov 25, 2019
#14721)

* Feature/read metrics from unix socket npipe (#14558)

Allow monitoring information from libbeat to be retrieved over unix domain socket and named pipes.

(cherry picked from commit baf8c1d)
@urso urso added the test-plan Add this PR to be manual test plan label Jan 16, 2020
@ph
Copy link
Contributor Author

ph commented Jan 24, 2020

@fearful-symmetry

Easiest way to test this is to have metricbeat's beat module configuration with the following:
tresting notes

- module: beat
  metricsets:
    - stats
    - state
  period: 10s
  hosts: ["http+npipe://./pipe/custom"]

And have a beat with monitoring enabled to publish it to the same named pipe:

http.enabled: true
http.host: npipe:///custom

@ph
Copy link
Contributor Author

ph commented Jan 24, 2020

After creating the above. I saw that the syntax are indeed a bit different and I've created #15832

@fearful-symmetry fearful-symmetry added test-plan-ok This PR passed manual testing and removed test-plan Add this PR to be manual test plan labels Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants