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

[Docker Plugin] add server hostname for each docker measurements #1599

Merged
merged 5 commits into from
Sep 6, 2016
Merged

[Docker Plugin] add server hostname for each docker measurements #1599

merged 5 commits into from
Sep 6, 2016

Conversation

aaronjheng
Copy link
Contributor

@aaronjheng aaronjheng commented Aug 8, 2016

Required for all PRs:

  • CHANGELOG.md updated
  • Sign CLA (if not already signed)

@sparrc
Copy link
Contributor

sparrc commented Aug 8, 2016

@aaronjheng I'm not sure this is a good idea. By default docker randomizes the name of the container, which means that every launched container will have a new name and the potential series cardinality is infinite.

When talking about influxdb, the database can't handle an infinite number of possible tag/value combinations.

You will need to workaround this using docker labels or other means.

@sparrc sparrc closed this Aug 8, 2016
@aaronjheng
Copy link
Contributor Author

@sparrc Thanks for replying. What I want to record is the host of docker engine, not the container.

@sparrc
Copy link
Contributor

sparrc commented Aug 8, 2016

I see, maybe I'm misunderstanding something then, is that different than the built-in telegraf host tag? Can you explain exactly what info.Name is? linking to some documentation would be extra helpful

@sparrc sparrc reopened this Aug 8, 2016
@aaronjheng
Copy link
Contributor Author

is that different than the built-in telegraf host tag?

Yes. In my use case, the telegraf program and the target docker-engine are in different servers. So the built-in telegraf host tag is not the docker-engine's host.

Can you explain exactly what info.Name is?

I can't find any specific description about this Info struct. But after digging some source code. I find this https://github.com/docker/docker/blob/master/daemon/info.go:

hostname := ""
if hn, err := os.Hostname(); err != nil {
    logrus.Warnf("Could not get hostname: %v", err)
} else {
    hostname = hn
}
v.Name = hostname

So, I assume that info.Name means the hostname of the docker engine's host.

@aaronjheng
Copy link
Contributor Author

Ping

@sparrc
Copy link
Contributor

sparrc commented Sep 5, 2016

Looks good to me, but can you make the tag more descriptive? I'd suggest docker_engine_host but I'll leave it up to your judgement

@sparrc sparrc added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 5, 2016
@aaronjheng
Copy link
Contributor Author

Sounds great! In docker context, engine_host is enough.

@sparrc sparrc merged commit 49ea4e9 into influxdata:master Sep 6, 2016
jackzampolin pushed a commit that referenced this pull request Oct 7, 2016
* add server hostname for each docker measurements

* update CHANGELOG

* move feature to v1.1

* tweak docker_engine_host tag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants