search

jmxtrans / jmxtrans-agent

Java Agent based JMX metrics exporter.
MIT License
179 stars 110 forks source link

stop writing host to Datadog StatsD tags #141

Closed devmage closed 5 years ago

devmage commented 5 years ago

Datadog documentation suggests that the host tag key is reserved, and is automatically assigned by the Datadog Agent. Users seeking to control its value should do so in the Agent config datadog.yaml file.

This removes the behavior that sets host in StatsDOutputWriter, and removes the hostname configuration parameter as it is no longer used.

This is motivated by some odd behavior that teammates and I saw with this code, where e.g. the hostname inside a Docker container may not be the hostname of the device running the container. Seems better in such a case to let the Datadog Agent figure out what the hostname should be, rather than trying to guess on its behalf, as metrics will then be correlated from the point of view of the agent that ingested the stat rather than the POV of the container that generated the stat.

2rs2ts commented 5 years ago

It can be useful to override host if you are sending to an agent on another machine and you don't want to lose the origin of the metrics. But in that case you can easily add the host tag on your own in your xml config. So this PR is a good change.

devmage commented 5 years ago

I'm a bit confused as to why removing the extraneous jenv file caused a build failure here, however I'm pleased that the build passed before it. 🤷‍♀

devmage commented 5 years ago

@cyrille-leclerc Any particular thoughts on this change?

cyrille-leclerc commented 5 years ago

Sorry for the late answer. I don't use DataDog, I don't have an opinion on this. Could a configuration flag solve both needs?

2rs2ts commented 5 years ago

@cyrille-leclerc since you can add the host tag in your own xml config that is already solved.

devmage commented 5 years ago

Yep! As @2rs2ts points out, a user could gain back the host by dynamically interpolating in a value to their host's jmxtrans-agent.xml, such as in the example for the InfluxDbOutputWriter: <tags>#hostname#</tags>.

@cyrille-leclerc

Could a configuration flag solve both needs?

A configuration flag could solve the need, yes. I believe a cleaner pattern would be to allow users to add a host where they need such. The trouble experienced here is that there is no current way in the Datadog writer to not emit a host field where one is not needed or desired. We can't turn it off. 😄

What do you think?

cyrille-leclerc commented 5 years ago

Understood @devmage, I merged your pull request, thank you very much and sorry for the delay. I am checking if additional docs can help the migration.