Add the ability to add tags to an elasticsearch instance #790
Conversation
| if config_url is None: | ||
| raise Exception("An url must be specified") | ||
|
|
||
| tags = ['url:%s' % config_url] | ||
| for tag in added_tags: |
There was a problem hiding this comment.
Won't this give a null pointer exception if tags isn't specified?
|
You're correct, I had not thought of that when I originally made the change. I've made an update and retested and it won't fail with NoneType anymore |
|
Thanks @clly ! |
|
@remh I did a rebase then a pull and merge. I looks like the conflicts fixed up correctly and it should cleanly merge with your upstream now. Just waiting on the Travis-ci build to finish. |
|
Thanks @clly The thing you should do is: Sorry about that. |
The tags key is taken from the instance and each entry appended to the already existing tags variable
|
Took some finagling, but I fix the branch. |
|
Thanks a lot! |
Add the ability to add tags to an elasticsearch instance
| @@ -148,6 +149,9 @@ def check(self, instance): | |||
|
|
|||
| # Tag by URL so we can differentiate the metrics from multiple instances | |||
| tags = ['url:%s' % config_url] | |||
| if added_tags is not None: | |||
| for tag in added_tags: | |||
| tags.append(tag) | |||
There was a problem hiding this comment.
I think this will keep adding the tags over and over again for each loop and leak memory. Maybe we should pass a copy of instance into the check method to avoid this bug in all the checks
There was a problem hiding this comment.
tagsis reset the line before, so it shouldn't happen.
There was a problem hiding this comment.
I think the case you are thinking about would have been
added_tags.append('url:%s' % config_url')at each iteration of the loop
There was a problem hiding this comment.
oh yeah, true, didn't see that the tags variable was different, nm
The tags key is taken from the instance and each entry appended to the
already existing tags variable. From my testing the single quotes are necessary