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

Kubernetes autodiscover - multiple ports #15796

Closed
anyasabo opened this issue Jan 23, 2020 · 10 comments · Fixed by #19398
Closed

Kubernetes autodiscover - multiple ports #15796

anyasabo opened this issue Jan 23, 2020 · 10 comments · Fixed by #19398
Labels
candidate Candidate to be added to the current iteration containers Related to containers use case enhancement Team:Platforms Label for the Integrations - Platforms team

Comments

@anyasabo
Copy link
Contributor

anyasabo commented Jan 23, 2020

Describe the enhancement:
For ECK we have multiple exposed ports on our Elasticsearch containers -- http and transport. When using autodiscover, it would be helpful to be able to specify which port we want to use for metricbeat. @exekias did some looking for me, and currently $data.port sets up events for all of the defined ports, but the transport one will not work and generate errors. If we could specify which port we want to use in our autodiscover configuration it would be helpful.

Maybe referencing the name of the port (e.g. $data.ports.http), the index of the port ($data.ports[1]), or maybe with a hint telling autodiscover which port name to use (co.elastic.metrics/metrics_port: http)?

@kaiyan-sheng kaiyan-sheng added the Team:Platforms Label for the Integrations - Platforms team label Jan 28, 2020
@exekias
Copy link
Contributor

exekias commented Jan 29, 2020

This is a fair request, thank you for opening!

The issue comes from the fact that we spawn an autodiscover event for every port in the Pod. Configs like ${data.host}:${data.port} would end up in several instances of the module, which is probably not what the user wants.

I think offering data.ports in the events makes sense, as it would allow users to pin the port they are interested on. I'm leaning towards allowing for named ports only, through ${data.ports.<port_name>}. Port names are guaranteed to unique per Pod. Unnamed ports would not be available in that dict.

thoughts @odacremolbap @ChrsMark @vjsamuel ?

@ChrsMark
Copy link
Member

+1 for the ${data.ports.<port_name>} approach. Sounds reasonable.

@exekias
Copy link
Contributor

exekias commented Mar 12, 2020

@anyasabo would named ports only be useful for you? I'm worried about using the index number for this, as a slight change can result on unexpected behaviors

@ChrsMark
Copy link
Member

@exekias @anyasabo wondering if this one is still valid after the changes happened at #18979 and #19052.

cc: @vjsamuel @jsoriano

@anyasabo
Copy link
Contributor Author

Ah sorry I missed your earlier question @exekias , yes specifying the named port would be helpful.

@ChrsMark if I understand the description correctly, the first PR linked only changes behavior when ports are not exposed. In this case multiple ports are exposed but we only want to scrape one.

Ditto with the second one it's definitely possible I'm misunderstanding, but it looks like that fixes a bug in how we were doing substring matching (e.g. 80 matched port 8000), so I also don't think that helps here.

@ChrsMark
Copy link
Member

👍 @anyasabo thanks ! I will provide a patch to support ${data.ports.<port_name>} too 🙂 .

@jsoriano
Copy link
Member

If the port is known, setting it in the configuration, without using data.port, should work, something like this:

co.elastic.metrics/module: elasticsearch
co.elastic.metrics/hosts: ${data.host}:9200

I think that this recent fix #18979 by @vjsamuel will prevent that configurations like these ones start unexpected modules, so this could work to avoid starting a module for the transport port in this case.

In any case having support for ${data.ports.<port_name>} sounds good.

@vjsamuel
Copy link
Contributor

You should just be able to do ${data.host}:8080 and it should work out of the box right? If you have multiple configurations on the container then you can use the multiple hints support that was recently merged as well.

@ChrsMark
Copy link
Member

What @jsoriano and @vjsamuel mention, using directly sth like ${data.host}:8080, was what made me thinking about this one initially.

However I think that what @anyasabo requests is slightly different and it comes to @jsoriano 's In any case having support for ${data.ports.<port_name>} sounds good.

In any case I drafted sth for the shake of this discussion: #19398

@ChrsMark
Copy link
Member

ChrsMark commented Jul 7, 2020

Closed via #19398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate Candidate to be added to the current iteration containers Related to containers use case enhancement Team:Platforms Label for the Integrations - Platforms team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants