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

Add support for port mapping in docker hints #31243

Merged
merged 8 commits into from
Apr 12, 2022

Conversation

ChrsMark
Copy link
Member

What does this PR do?

Add support for port mapping in docker autodiscover. The idea is similar to what we do for Kubernetes which was added at #19398.

The idea is to make it possible for collecting metrics using the HostPort of the container as described at #19825, by referring to the proper internal port of the container (see manual testing notes for an example).

Why is it important?

To cover cases like #19825.

How to test this PR locally

  1. Use the following configuration:
metricbeat.autodiscover:
  providers:
    - type: docker
      hints.enabled: true

output.console:
  pretty: true
  1. Run metricbet with ./metricbeat -e -d "module" -c metricbeat.yml
  2. Start a target container with docker run --name nats --rm -p 4222:4222 -p 8081:8222 --label co.elastic.metrics/module="nats" --label co.elastic.metrics/metricsets="stats" --label co.elastic.metrics/hosts='localhost:${data.ports.8222}' nats:2.1.4 --http_port 8222
  3. Verify in the console output that Metricbeat is collecting metrics properly from localhost:8222 endpoint.

Related issues

@ChrsMark ChrsMark requested review from jsoriano and a team April 11, 2022 11:00
@ChrsMark ChrsMark requested a review from a team as a code owner April 11, 2022 11:00
@ChrsMark ChrsMark self-assigned this Apr 11, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 11, 2022
@botelastic
Copy link

botelastic bot commented Apr 11, 2022

This pull request doesn't have a Team:<team> label.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Nice. LGTM.

Please add a changelog entry.

And would it be possible to add tests for this?

@ChrsMark
Copy link
Member Author

Nice. LGTM.

Please add a changelog entry.

And would it be possible to add tests for this?

Yeap, I'm planning to add docs/changelog and good idea adding tests too!

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 11, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-04-12T12:44:02.021+0000

  • Duration: 83 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 22299
Skipped 1935
Total 24234

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@@ -43,7 +43,7 @@ func NewClient() (Client, error) {
}

// ContainerStart pulls and starts the given container
func (c Client) ContainerStart(image string, cmd []string, labels map[string]string) (string, error) {
func (c Client) ContainerStart(image string, cmd []string, labels map[string]string, hostConfig *container.HostConfig) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit. Consider defining new method to keep backwards compatibility. Though probably nothing is using this method outside of beats.

Suggested change
func (c Client) ContainerStart(image string, cmd []string, labels map[string]string, hostConfig *container.HostConfig) (string, error) {
// ...
type StartOptions struct {
HostConfig *container.HostConfig
}
// ...
func ContainerStart(image string, cmd []string, labels map[string]string) (string, error) {
return ContainerStartWithOptions(image, cmd, labels, StartOptions{})
}
// ...
func (c Client) ContainerStartWithOptions(image string, cmd []string, labels map[string]string, options StartOptions) (string, error) {
...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! This test is not correct nevertheless so I will revert this. Unfortunately we don't have proper tests at the moment that I could extend to cover this feature addition.

Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Signed-off-by: chrismark <[email protected]>
Copy link
Contributor

@MichaelKatsoulis MichaelKatsoulis left a comment

Choose a reason for hiding this comment

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

LGTM

@ChrsMark ChrsMark merged commit fea8a70 into elastic:main Apr 12, 2022
v1v added a commit to v1v/beats that referenced this pull request Apr 18, 2022
…er-tar-gz

* upstream/main: (139 commits)
  [Automation] Update elastic stack version to 8.3.0-c655cda8 for testing (elastic#31322)
  Define a queue metrics reporter interface  (elastic#31289)
  [Oracle Module] Change tablespace metricset collection period (elastic#31259)
  libbeat/reader/syslog: relax timestamp parsing to allow leading zero (elastic#31254)
  [Automation] Update elastic stack version to 8.3.0-55ba6f37 for testing (elastic#31311)
  [libbeat] Remove unused fields and functions in the memory queue (elastic#31302)
  [libbeat] Cleaning up some unneeded helper types (elastic#31290)
  Readme for kibana module (elastic#31276)
  [Automation] Update elastic stack version to 8.3.0-4be61f32 for testing (elastic#31296)
  x-pack/winlogbeat/module/routing/ingest: fix typo for channel name (elastic#31291)
  Small pipeline cleanup removing some unused data fields (elastic#31288)
  removing info log (elastic#30971)
  Simplify TLS config deserialization (elastic#31168)
  Detect new files under known paths in filestream input (elastic#31268)
  Add support for port mapping in docker hints (elastic#31243)
  Update qa-labels.yml (elastic#31260)
  libbeat: log debug for `proxy_url` and fixed docs (elastic#31130)
  [heartbeat][docs] Add note about ensuring correct index settings for uptime (elastic#31146)
  [Automation] Update elastic stack version to 8.3.0-2c8f9574 for testing (elastic#31256)
  [Filebeat] fix m365_defender pipeline bug (elastic#31227)
  ...
kush-elastic pushed a commit to kush-elastic/beats that referenced this pull request May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 candidate autodiscovery needs_team Indicates that the issue/PR needs a Team:* label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metricbeat: Hint based autodiscover for docker does not work when internal and exposed port differ
4 participants