Docker module cleanup #2665

ruflin · 2016-10-03T19:47:14Z

Make default period 10s, same as for all other beats
Remove unnecessary code comments
Make convertContainerPort local to container MetricSet
Only add labels to container MetricSet if not empty
Apply some code conventions
Convert local @timestamp to timestamp to prevent confusion
Replace rx / tx with in / out in network metricset to be consistent with system network metricset
Add interface name as field instead of having it as a dynamic element. This makes it easier to filter.
If not containers are running, just no events are returned
Add data.json for all metricsets
Introduce ToMapStr for Container data
Move socket to Container MapStr. Is Socket info really needed?
Remove unnecessary nesting levels as namespace already given by metricset
Have one docker client per metricset instead of having one global client

ruflin · 2016-10-03T19:48:15Z

@douaejeouit It would be nice if you could have a look at this PR and especially have a look at the // TODO I added. Would be good to get your thoughts on these.

tsg · 2016-10-03T20:29:31Z

metricbeat/module/docker/container/data.go


 	event := common.MapStr{
-		"@timestamp": time.Now(),
+		"timestamp": time.Now(),


Not sure I understand this, does it mean we'll have both @timestamp and timestamp? Seems unnecessary.

@tsg Agree. Before one was overwriting the other (or at least it tried and didn't as it was not common.Time). With the above implementation each event can have its separate timestamp instead of one for all events of this batch. But as the heavy work happens when the info from the docker api is fetched and the timestamp is generated afterwards, I suggest to just remove it.

Perhaps @douaejeouit can share some insights here?

@tsg The @ timestamp here shouldn't be time.now() ! The idea is to get the time generate by Docker daemon while sending the events requested ( here the containers event), this is what I did for all the other metricsets.
I will be on it tomorrow morning! There is some smalll changes to do, otherwise, we can simply keep it as it is for the containers event! What do you think guys ? @ruflin @tsg

@douaejeouit I saw this part, but we are talking here about a milli second difference in time. I don't think it is worth to do this and have the logic because of this different from all other modules / metricsets where the timestamp represents when the event fetching started. The nice part of this is that timestamp + rrt is when the even creation ended. With the above implementation it is not 100% clear to me what the timestamp will exactly represent.

@douaejeouit Perhaps you can share some insights on what the timestamp exactly is.

@ruflin Ok!
But .. what do you mean by having one dockerclient per metricset instead of having one global client? For me the docker client instance should be created only once. Could you please clarify this point?
Concerning timestamp, as I said before, it's overwritten and it represents the time when the stats were read from the deamon. But since it's only a matter of a milli seconds difference in time, I agree with you! We can change the logic 👍

The previous implementation didn't allow to have two different docker host machines as only one global client was used. In general MetricSets are designed to no share information with each other. So in case one MetricSet has some issues, it doesn't affect the others. This should also be true for the client connection, means every MetricSet should have its own instance of the client.

The downside of this is when using all docker module MetricSets, 5 connections are open to the Docker host instead of one. Currently I would say this is acceptable, but in case this becomes a problem we will tackle it. There are options like making the Module more powerful so multiple MetricSets in one Module, meaning if in the config multiple metricsets are defined in a module, this module could also be used to share some information. This will potentially require some refactoring as it exceeds the current capability of the interface methods of the module interface.

Ok,
Indeed, I made the choice to have one client for failing to 5 simultaneous connections if all the metricsets are used.. anyway, it's Ok as it is not problematic.

Before it was not only 1 client for all 5 metricsets, but one client for all docker modules (one docker module can be used multiple times in the config) which broke the implementation.

All right .. I can see now! Thanks

tsg · 2016-10-04T09:04:56Z

LGTM.

@timestamp

* Make default period 10s, same as for all other beats * Remove unnecessary code comments * Make convertContainerPort local to container MetricSet * Only add labels to container MetricSet if not empty * Apply some code conventions * Convert local @timestamp to timestamp to prevent confusion * Replace rx / tx with in / out in network metricset to be consistent with system network metricset * Add interface name as field instead of having it as a dynamic element. This makes it easier to filter. * If not containers are running, just no events are returned * Add data.json for all metricsets * Introduce ToMapStr for Container data * Move socket to Container MapStr. Is Socket info really needed? * Remove unnecessary nesting levels as namespace already given by metricset * Have one docker client per metricset instead of having one global client

ruflin · 2016-10-04T12:27:01Z

@douaejeouit We merged this PR, but lets continue this discussion about the timestamp. Happy to readd it if needed. Please also have a look at the PR to check if I changed some code that should not be changed.

douaejeouit · 2016-10-05T12:23:27Z

metricbeat/module/docker/cpu/cpu_test.go

@@ -73,6 +74,8 @@ func TestCPUService_UsageInUsermode(t *testing.T) {
 	//  THEN
 	assert.Equal(t, float64(0.50), result)
 }
+
+/* TODO: uncomment


@ruflin Should I comment it? This function needs to be updated in order to match with last changes you made. I'll do it.

Yes, please. Thanks

douaejeouit · 2016-10-05T12:25:50Z

metricbeat/module/docker/helper.go

+		"id":   c.Id,
+		"name": c.Name,
+		// TODO: Is this really needed
+		"socket": GetSocket(),


It's an extra information. We can ignore it if you judge it isn't needed!

I'm trying to understand the use case behind it. It see that people will search for id and name, but will the socket information be of any use? I think this is useful in the info metricset, but it is required in the other metricsets. I'm hesitant to remove it as I'm sure there was a good reason to add it which I'm just not aware of yet.

The main objectif of having it as a field is to have an clear idea about the conf and to check it as well. Espacially in case we have two docker modules running and each one listen on different socket. Since we can bind docker to another host or port using a tcp connection, it's always good to know if it listen on TCP or a Unix socket or both, that's why it's present n all the metricsets!

I'm thinking in case a socket is used, if we should copy over the socket information to metricset.host info instead?

+1, it sounds good for me!

Added a task to the list here: #2629

douaejeouit · 2016-10-05T12:43:09Z

metricbeat/module/docker/helper.go

 	i := 0
 	var keys []string
 	for key := range labels {
 		keys = append(keys, key)
 	}
 	sort.Strings(keys)
 	for _, k := range keys {
+		// Replace all . in the labels by _
+		// TODO: WHY?


It was the naming used in dockbeat Project, where it has been decided to format the labels output . I didn't want it to be different, that's why I left it at it is. If it's not necessery. We can leave it as it's generated by docker's API.

I was first thinking . were replaced by _ because in ES <2.4 . can't be used in field names. But it seems to be the names where it is replaced? Perhaps there are some disadvantages in Kibana with the . ? Would be interesting to get some more background info on this decision.

Indeed, it was due to some ES & Kibana issues in some previous version. Apparently, Kibana couldn't distinguish between fields whose names contains a . and those who are nested under a different objects! So as a temporary solution, we replaced the feild names containing ( . ) by ( _ ) to avoid any potential problem. By the way, I'm wondering if the bug was fixed. If so, we can ignore this function and keep our field names without any modification.

I wasn't aware, that the above was and issue for values. I would expect it to be an issue for keys, but also values? Were the values defined as text or keywords? That could have been the issue, that if it was not defined as keywors that the tokenizer was applied? But this can be solved by applying the right mapping.

We should test the behaviour here, as in general I think it would be nice to keep the name of the labels the same as the user set it, as this is what he will probably look for.

If I'm not mistaken, it concerns only keys !
+1 for not changing the name of labels

Added a task to the list here: #2629

rikatz · 2016-11-20T20:20:10Z

Guys, with my previous contribution #3024 about labels and dots, is this still necessary?

ruflin · 2016-11-21T07:48:14Z

@rikatz There were quite a lot of changes and discussions in this PR. Which part are you referring to?

rikatz · 2016-11-21T10:57:04Z

@ruflin Sorry! about the labels cleanup, as mentioned on the helper.go here, about the replacement of dots to underlines.

Also, the task "Change and test labels to be taken with . if possible: #2665 (comment)" on #2629

ruflin · 2016-11-21T13:13:51Z

@rikatz I checked the checkboxes in the issue as done.

ruflin added in progress Pull request is currently in progress. review Metricbeat Metricbeat labels Oct 3, 2016

ruflin force-pushed the docker-module-cleanup branch from 9a4abde to 4cdcb27 Compare October 3, 2016 20:13

tsg reviewed Oct 3, 2016

View reviewed changes

ruflin force-pushed the docker-module-cleanup branch 3 times, most recently from fbb350a to 896d480 Compare October 4, 2016 08:52

ruflin removed the in progress Pull request is currently in progress. label Oct 4, 2016

ruflin force-pushed the docker-module-cleanup branch from 896d480 to 83e7c61 Compare October 4, 2016 11:26

ruflin mentioned this pull request Oct 4, 2016

Docker module improvements #2629

Closed

19 tasks

tsg merged commit f9effaf into elastic:master Oct 4, 2016

ruflin deleted the docker-module-cleanup branch October 4, 2016 12:27

douaejeouit reviewed Oct 5, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docker module cleanup #2665

Docker module cleanup #2665

ruflin commented Oct 3, 2016 •

edited

Loading

ruflin commented Oct 3, 2016

tsg Oct 3, 2016

ruflin Oct 4, 2016

douaejeouit Oct 4, 2016 •

edited

Loading

ruflin Oct 4, 2016

douaejeouit Oct 5, 2016

ruflin Oct 5, 2016

douaejeouit Oct 5, 2016

ruflin Oct 5, 2016

douaejeouit Oct 5, 2016

tsg commented Oct 4, 2016

ruflin commented Oct 4, 2016

douaejeouit Oct 5, 2016 •

edited

Loading

ruflin Oct 5, 2016

douaejeouit Oct 5, 2016

ruflin Oct 5, 2016

douaejeouit Oct 6, 2016

ruflin Oct 6, 2016

douaejeouit Oct 6, 2016

ruflin Oct 6, 2016

douaejeouit Oct 5, 2016 •

edited

Loading

ruflin Oct 5, 2016

douaejeouit Oct 6, 2016 •

edited

Loading

ruflin Oct 6, 2016

douaejeouit Oct 6, 2016 •

edited

Loading

ruflin Oct 6, 2016

rikatz commented Nov 20, 2016 •

edited

Loading

ruflin commented Nov 21, 2016

rikatz commented Nov 21, 2016

ruflin commented Nov 21, 2016

Docker module cleanup #2665

Docker module cleanup #2665

Conversation

ruflin commented Oct 3, 2016 • edited Loading

ruflin commented Oct 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douaejeouit Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsg commented Oct 4, 2016

ruflin commented Oct 4, 2016

douaejeouit Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douaejeouit Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douaejeouit Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douaejeouit Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rikatz commented Nov 20, 2016 • edited Loading

ruflin commented Nov 21, 2016

rikatz commented Nov 21, 2016

ruflin commented Nov 21, 2016

ruflin commented Oct 3, 2016 •

edited

Loading

douaejeouit Oct 4, 2016 •

edited

Loading

douaejeouit Oct 5, 2016 •

edited

Loading

douaejeouit Oct 5, 2016 •

edited

Loading

douaejeouit Oct 6, 2016 •

edited

Loading

douaejeouit Oct 6, 2016 •

edited

Loading

rikatz commented Nov 20, 2016 •

edited

Loading