-
Notifications
You must be signed in to change notification settings - Fork 46
Use config-based Docker endpoint instead of hardcoded #62
Use config-based Docker endpoint instead of hardcoded #62
Conversation
@kindermoumoute what would be the best way to stub |
return nil, err | ||
} | ||
|
||
err = d.init_client(conf["endpoint"]) |
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.
This way you are going to initialize new client for each call to CollectMetrics
. I would suggest to do it only once. (check for nil
?)
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.
@marcin-krolik take a look at new commit, in initClient
method, we check if client was initialized, if yes (checking if .client
is not nil
, but i think it would be more useful to do some sort of ping
check if Docker daemon is alive and its API is responsive).
@marcin-krolik your comment was addressed with 8358759. |
@denismakogon Thanks, I think it's better now.
I also think this way. I don't know if you read my comment to your previous PR, just to make sure it was not missed - In current sprint I'm working on heavy refactor of docker collector. There will be a lot of changes, like getting rid of expensive collects (only collecting requested metrics), proper discovery of cgroupfs driver, pinging docker deamon as well :) |
@marcin-krolik yes, i've seen that. Probably i can help with that. |
So, @kindermoumoute @marcin-krolik i just tried to build and run this plugin using this PR, it seems like it works fine. At least i'm able to create task and see metrics and some data published into a file (using file publisher). Would this mean that plugin works? Can someone confirm that it worked for you too? |
return err | ||
} | ||
d.client = dc | ||
return nil |
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.
this line seems unnecessary as there seems nothing else besides if statement.
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.
Agreed, removing.
return &docker{ | ||
containers: map[string]containerData{}, | ||
client: dc, | ||
client: nil, |
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.
if the client is nil at New(), do you still want to keep it in the type?
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.
Agreed, removing.
@@ -66,48 +66,56 @@ var mockMts = []plugin.Metric{ | |||
Namespace: plugin.NewNamespace(NS_VENDOR, NS_PLUGIN). | |||
AddDynamicElement("docker_id", "an id of docker container"). | |||
AddStaticElements("spec", "creation_time"), | |||
Config: plugin.Config{"endpoint": "unix:///var/run/docker.sock"}, |
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.
probably a visible scope variable instead of the same definition everywhere?
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.
Agreed, refactoring.
@denismakogon, thanks for your contribution. |
Use config-based Docker endpoint instead of hardcoded one