-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Telegraph Docker Input Plugin : per docker running status support #4259
Telegraph Docker Input Plugin : per docker running status support #4259
Conversation
plugins/inputs/docker/docker.go
Outdated
@@ -435,6 +435,22 @@ func (d *Docker) gatherContainer( | |||
} | |||
} | |||
} | |||
if info.State != nil { | |||
statefields := map[string]interface{}{ | |||
"status": info.State.Status, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have this value as a tag on all docker_container_*
measurements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this be too much of duplication since the same information is across multiple measurements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a little bit of duplication, but it seems useful to be able to "group by" status on any of the container measurements.
Side note: If I could do it all over from scratch I would just have a single docker_container
measurement, but now it's not worth the disruption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the code to add status for each container . It can get tricky for cases like CPU(where there is a per-cpu option) and Network(where there is a per interface option). Also since these collectors are all over the place , we would have to make a separate "docker inspect" API call at each of these places . It seems a bit inefficient considering how it currently is designed . Would you still like me to do it that way?
To handle the "group by" feature you requested , we could plug in "container_id" into this which allows you to match with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, not if we need to make extra API calls. Let's just add it to the new measurement as a tag and we can try to fish it through to other measurements later on, if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this one ! I've added it as a tag.. So now it comes not only for docker_container_status , but also for cpu,mem etc
Attached some logs :
docker_container_status,container_image=ubuntu,container_name=vibrant_benz,container_status=running,container_version=unknown,deviceid=mydeviceid,engine_host=raspberrypi,server_version=18.05.0-ce started_at="2018-06-14T05:48:53.266176036Z",finished_at="0001-01-01T00:00:00Z",oomkilled=false,pid=5278i,exitcode=0i 1528956922000000000
docker_container_mem,container_image=ubuntu,container_name=vibrant_benz,container_status=running,container_version=unknown,deviceid=mydeviceid,engine_host=raspberrypi,server_version=18.05.0-ce limit=0i,usage=0i,max_usage=0i,usage_percent=0,container_id="f85ef98b08bde3ad6a93fadcacfa33a57a1f833119cce0309cadb15c45df5cf4" 1528956921000000000
docker_container_cpu,container_image=ubuntu,container_name=vibrant_benz,container_status=running,container_version=unknown,cpu=cpu-total,deviceid=mydeviceid,engine_host=raspberrypi,server_version=18.05.0-ce usage_total=102424740i,usage_in_usermode=60000000i,usage_system=10927430000000i,throttling_periods=0i,throttling_throttled_time=0i,usage_in_kernelmode=20000000i,throttling_throttled_periods=0i,container_id="f85ef98b08bde3ad6a93fadcacfa33a57a1f833119cce0309cadb15c45df5cf4",usage_percent=0 1528956921000000000
docker_container_cpu,container_image=ubuntu,container_name=vibrant_benz,container_status=running,container_version=unknown,cpu=cpu0,deviceid=mydeviceid,engine_host=raspberrypi,server_version=18.05.0-ce usage_total=7975104i,container_id="f85ef98b08bde3ad6a93fadcacfa33a57a1f833119cce0309cadb15c45df5cf4" 1528956921000000000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit tests seem to be failing after adding "container_status" as a tag . I'm not quite sure how to deal with this. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed this . All tests pass now.
plugins/inputs/docker/docker.go
Outdated
"status": info.State.Status, | ||
"running": info.State.Running, | ||
"paused": info.State.Paused, | ||
"restarting": info.State.Restarting, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to save the running, paused, restarting, dead values, since this is all encompassed by the status field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point . I will remove them....
plugins/inputs/docker/docker.go
Outdated
"exitcode": info.State.ExitCode, | ||
"error": info.State.Error, | ||
"startedat": info.State.StartedAt, | ||
"finishedat": info.State.FinishedAt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if error, startedat, finishedat are interesting enough to include, I feel like many people will want to exclude them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel startedat,finishedat is important since it gives useful information on the uptime of the container or even how long ago the container died(based on "status") . If you still think its not needed , we can take it off!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding error . I will remove it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, lets just rename them started_at
and finished_at
and convert them to unixnano format (unix epoch timestamp in nanosecond precision). This is the format we have been using recently for encoding timestamps into a field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've renamed this to started_at and finished_at .
I have a query regarding converting the time from RFC3339 format (Example : 2018-06-14T05:48:53.266176036Z ) to Unix Nano format . Golang has the option to convert using the following code :
t1, err := time.Parse(time.RFC3339,"0001-01-01T00:00:00Z")
and then use
t1.UnixNano()
Here's some sample output of the timestamps :
docker_container_status,container_image=ubuntu,container_name=vibrant_benz,container_status=running,container_version=unknown,deviceid=mydeviceid,engine_host=raspberrypi,server_version=18.05.0-ce started_at="2018-06-14T05:48:53.266176036Z",finished_at="0001-01-01T00:00:00Z",oomkilled=false,pid=5278i,exitcode=0i 1528956922000000000
In the above case the docker is still running so it has a valid start time , although its endtime is invalid which is "0001-01-01T00:00:00Z" . If i use the above go code to convert this to unix nano , I get "-6795364578871345152" . As you can see it returns a long negative invalid value . Would this be okay? How else would you like me to handle this ?If this is fine then I will go ahead and commit this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to use time.IsZero(), and if true skip adding the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! I've done that.
I updated the README in c98b58d |
* origin: (39 commits) Document path tag in tail input Update changelog Added path tag to tail input plugin (influxdata#4292) Run windows tests with -short Fix postfix input handling of multi-level queues (influxdata#4333) Update changelog Add support for comma in logparser timestamp format (influxdata#4311) Update vendoring to dep from gdm (influxdata#4314) Update changelog Add new measurement with results of pgrep lookup to procstat input (influxdata#4307) Update changelog Add valuecounter aggregator plugin (influxdata#3523) Update changelog Update docker input documentation for container status Add container status tag to docker input (influxdata#4259) Drop CI support for Go 1.8 (influxdata#4309) Update changelog Fix selection of tags under nested objects in the JSON parser (influxdata#4284) Update changelog Add owner tag on partitions in burrow input (influxdata#4281) ...
Hi, I am unable to find the telegraf.conf
Docker version
|
@Puneeth-n This feature will be included in Telegraf 1.8.0, but if you are okay with a slightly higher chance of encountering bugs you can try using the nightly development builds to demo this feature now. |
Currently the docker input plugin does not provide the running status of the docker . It does provide health status of the docker but that is only retrieved if the docker application supports it . This enhancement extends the plugin to give out the running status of the docker which is taken from the Docker Container Inspect API.
Required for all PRs: