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

Updated dockerstatsreceiver to respect DOCKER_HOST env variable. #37589

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sonalgaud12
Copy link

Description

The dockerstatsreceiver previously defaulted to using unix:///var/run/docker.sock for the Docker endpoint. While the configuration allows overriding this, it did not automatically respect the DOCKER_HOST environment variable. This update ensures that the receiver resolves the endpoint in the following order:

  1. The value set in the YAML configuration.
  2. The DOCKER_HOST environment variable, if set.
  3. The fallback default unix:///var/run/docker.sock.

Link to tracking issue

Fixes #35779

Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

Can I also ask that you add some test to help validate the change going forward?
Mostly so the behaviour is captured as part of the tests.

Comment on lines 58 to 73
func resolveDockerEndpoint(cfg *docker.Config) {
// If endpoint is explicitly set in config, respect it
if cfg.Endpoint != "" {
return
}

// Check DOCKER_HOST environment variable
if dockerHost := os.Getenv(dockerHostEnv); dockerHost != "" {
cfg.Endpoint = dockerHost
return
}

// Fall back to default endpoint
cfg.Endpoint = defaultEndpoint
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mutating the config object, can you add this as a method to *Config that returns the expected value?

It should make it easier to test that way.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@sonalgaud12
Copy link
Author

Hi @MovieStoreGuy, I would appreciate your review. This is my first time writing a test, so I’d love your feedback on whether I made any mistakes or if there’s anything I can improve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Respect the environment variable DOCKER_HOST in resolving the endpoint
3 participants