)]}'
{"monasca_agent/collector/checks/check.py":[{"author":{"_account_id":2419,"name":"Roland Hochmuth","email":"roland.hochmuth@hpe.com","username":"roland-hochmuth-s"},"change_message_id":"fc35910da57f0fb06425bb2fb0a8402c272f8124","unresolved":false,"context_lines":[{"line_number":44,"context_line":"        self.library_versions \u003d None"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @classmethod"},{"line_number":47,"context_line":"    def factory(cls, name, init_config, agent_config, instances\u003dNone):"},{"line_number":48,"context_line":"        \"\"\"Creates a new check."},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"        The purpose of this factory method is to allow checkers to initialize"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aa08908_b571b4aa","line":47,"range":{"start_line":47,"start_character":8,"end_line":47,"end_character":15},"updated":"2016-06-07 21:01:17.000000000","message":"I commented on this in another review, but I think rather than introducing a new factory method, something like the code below would be more straight-forward and also address some of my concerns, such as having base class know about derived classes.\n\nimport sys\n\nclass CpuCheck:\n    def foo(self):\n        pass\n\nclass LinuxCpuCheck(CpuCheck):\n    def foo(self):\n        print \"LinuxStats\"\n\nclass Win32CpuCheck(CpuCheck):\n    def foo(self):\n        print \"Win32Stats\"\n\ndef Cpu():\n    if sys.platform \u003d\u003d \u0027win32\u0027:\n        return Win32CpuCheck()\n    else:\n        return LinuxCpuCheck()\n\na \u003d Cpu()\na.foo()","commit_id":"01fe4c2653c46856db134c4cc1da120dc8a579b8"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"7d14e6ece2ce148f172fb836b3b11b2ed081ea1c","unresolved":false,"context_lines":[{"line_number":44,"context_line":"        self.library_versions \u003d None"},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"    @classmethod"},{"line_number":47,"context_line":"    def factory(cls, name, init_config, agent_config, instances\u003dNone):"},{"line_number":48,"context_line":"        \"\"\"Creates a new check."},{"line_number":49,"context_line":""},{"line_number":50,"context_line":"        The purpose of this factory method is to allow checkers to initialize"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aa08908_91dffe0b","line":47,"range":{"start_line":47,"start_character":8,"end_line":47,"end_character":15},"in_reply_to":"7aa08908_b571b4aa","updated":"2016-06-15 07:47:31.000000000","message":"I see you\u0027re suggesting a factory instead of a factory method. It is an idea I\u0027ve considered, before deciding on the factory method. There are a couple of reasons I\u0027ve preffered the factory method:\n\nIndeed, base classes won\u0027t have to know about derived classes. But it is going to be harder to implement. Let\u0027s see how the checkers are currently initiated:\n\n    - for every file defined in the conf.d folder, monasca-agent will load it\u0027s respective checker module. For cpu.yaml conf.d file, monasca-agent will load the cpu checker module.\n    - for each loaded module, it will search for a subclass of AgentChecker via introspection.\n    - if a subclass is found, an instance will be created:\n        \n                c \u003d check_class(check_name, init_config\u003dinit_config,\n                                agent_config\u003dagent_config, instances\u003dinstances)\n\nThe problem with the current implementation, is that There\u0027s no guarantee which checker will be created (LinuxCpuChecker or Win32CpuChecker). Of course, the implementation can be changed to use the factory you\u0027ve suggested, although, for simplicity, it will have to be renamed to a more generic name a bit:\n\n    def get_checker(*args, **kwargs):\n        if sys.platform \u003d\u003d \u0027win32\u0027:\n            return Win32CpuChecker(*args, **kwargs)\n        else:\n            return LinuxCpuChecker(*args, **kwargs)\n\nThis way, when the checker is loaded, we should only look for the \u0027get_checker\u0027 function, instead of a AgentChecker subclass.\n\nNow, the ugly part: This would mean I\u0027d have to add 52 factories, one for each checker module. Most of them don\u0027t need any special implementation. Also, not sure if this is the case, if someone has some custom out-of-tree checkers, they won\u0027t have this factory, which means their checkers won\u0027t work.\n\nAlternatively, when we create the checker object, we can look for the \u0027get_checker\u0027 function. If found, use it; if not, just use the old behaviour.\n\nBut, IMO, having a factory method was the simplest way to move forward. It allows us to have a default implementation for it: Load.factory returns a Load checker instance, Memory.factory returns a Memory checker instance, etc. \n\nWe only needed to override the factory method when there are differerent implementations (process checker, cpu checker, etc.). The fact that it knows about its subclasses is the only negative side, which I don\u0027t see it as a big issue. It works fine.","commit_id":"01fe4c2653c46856db134c4cc1da120dc8a579b8"},{"author":{"_account_id":22157,"name":"Andrea Adams","email":"aadams@hpe.com","username":"andrea.adams"},"change_message_id":"db6d6298fb26785b80b63c0f80c8ed9dda39ca74","unresolved":false,"context_lines":[{"line_number":62,"context_line":"        if instances:"},{"line_number":63,"context_line":"            return cls(name, init_config, agent_config, instances)"},{"line_number":64,"context_line":"        else:"},{"line_number":65,"context_line":"            return cls(name, init_config, agent_config)"},{"line_number":66,"context_line":""},{"line_number":67,"context_line":"    def instance_count(self):"},{"line_number":68,"context_line":"        \"\"\"Return the number of instances that are configured for this check."}],"source_content_type":"text/x-python","patch_set":10,"id":"fa6399be_8e523612","line":65,"range":{"start_line":65,"start_character":12,"end_line":65,"end_character":55},"updated":"2016-10-11 21:36:08.000000000","message":"Can we set instances here to avoid some of the confusing code in util.py?","commit_id":"444c64dc2534b618f3611aa6c1f94e27eb10a59c"}],"monasca_agent/common/util.py":[{"author":{"_account_id":2419,"name":"Roland Hochmuth","email":"roland.hochmuth@hpe.com","username":"roland-hochmuth-s"},"change_message_id":"fc35910da57f0fb06425bb2fb0a8402c272f8124","unresolved":false,"context_lines":[{"line_number":572,"context_line":"                # instances argument in the constructor."},{"line_number":573,"context_line":"                c \u003d check_class(check_name, init_config\u003dinit_config,"},{"line_number":574,"context_line":"                                agent_config\u003dagent_config)"},{"line_number":575,"context_line":"                c.instances \u003d instances"},{"line_number":576,"context_line":"        except Exception as e:"},{"line_number":577,"context_line":"            log.exception(\u0027Unable to initialize check %s\u0027 % check_name)"},{"line_number":578,"context_line":"            traceback_message \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aa08908_75bd8cb6","line":575,"range":{"start_line":575,"start_character":30,"end_line":575,"end_character":39},"updated":"2016-06-07 21:01:17.000000000","message":"This just isn\u0027t a good way to do this type of initialization.","commit_id":"01fe4c2653c46856db134c4cc1da120dc8a579b8"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"7d14e6ece2ce148f172fb836b3b11b2ed081ea1c","unresolved":false,"context_lines":[{"line_number":572,"context_line":"                # instances argument in the constructor."},{"line_number":573,"context_line":"                c \u003d check_class(check_name, init_config\u003dinit_config,"},{"line_number":574,"context_line":"                                agent_config\u003dagent_config)"},{"line_number":575,"context_line":"                c.instances \u003d instances"},{"line_number":576,"context_line":"        except Exception as e:"},{"line_number":577,"context_line":"            log.exception(\u0027Unable to initialize check %s\u0027 % check_name)"},{"line_number":578,"context_line":"            traceback_message \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":3,"id":"7aa08908_41488ecc","line":575,"range":{"start_line":575,"start_character":30,"end_line":575,"end_character":39},"in_reply_to":"7aa08908_75bd8cb6","updated":"2016-06-15 07:47:31.000000000","message":"I\u0027ve only changed L567-569, not this.","commit_id":"01fe4c2653c46856db134c4cc1da120dc8a579b8"},{"author":{"_account_id":22157,"name":"Andrea Adams","email":"aadams@hpe.com","username":"andrea.adams"},"change_message_id":"db6d6298fb26785b80b63c0f80c8ed9dda39ca74","unresolved":false,"context_lines":[{"line_number":566,"context_line":"                c \u003d check_class.factory(check_name, init_config\u003dinit_config,"},{"line_number":567,"context_line":"                                        agent_config\u003dagent_config,"},{"line_number":568,"context_line":"                                        instances\u003dinstances)"},{"line_number":569,"context_line":"            except TypeError as e:"},{"line_number":570,"context_line":"                # Backwards compatibility for checks which don\u0027t support the"},{"line_number":571,"context_line":"                # instances argument in the constructor."},{"line_number":572,"context_line":"                c \u003d check_class.factory(check_name, init_config\u003dinit_config,"},{"line_number":573,"context_line":"                                        agent_config\u003dagent_config)"},{"line_number":574,"context_line":"                c.instances \u003d instances"},{"line_number":575,"context_line":"        except Exception as e:"},{"line_number":576,"context_line":"            log.exception(\u0027Unable to initialize check %s\u0027 % check_name)"},{"line_number":577,"context_line":"            traceback_message \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":10,"id":"fa6399be_ae09ba2e","line":574,"range":{"start_line":569,"start_character":0,"end_line":574,"end_character":39},"updated":"2016-10-11 21:36:08.000000000","message":"Is this all unneeded code since the new factory method will always accept these arguments?","commit_id":"444c64dc2534b618f3611aa6c1f94e27eb10a59c"},{"author":{"_account_id":8213,"name":"Claudiu Belu","email":"cbelu@cloudbasesolutions.com","username":"claudiub"},"change_message_id":"3f6540575a36daf1217ac3e3fe84ba688eb7543c","unresolved":false,"context_lines":[{"line_number":566,"context_line":"                c \u003d check_class.factory(check_name, init_config\u003dinit_config,"},{"line_number":567,"context_line":"                                        agent_config\u003dagent_config,"},{"line_number":568,"context_line":"                                        instances\u003dinstances)"},{"line_number":569,"context_line":"            except TypeError as e:"},{"line_number":570,"context_line":"                # Backwards compatibility for checks which don\u0027t support the"},{"line_number":571,"context_line":"                # instances argument in the constructor."},{"line_number":572,"context_line":"                c \u003d check_class.factory(check_name, init_config\u003dinit_config,"},{"line_number":573,"context_line":"                                        agent_config\u003dagent_config)"},{"line_number":574,"context_line":"                c.instances \u003d instances"},{"line_number":575,"context_line":"        except Exception as e:"},{"line_number":576,"context_line":"            log.exception(\u0027Unable to initialize check %s\u0027 % check_name)"},{"line_number":577,"context_line":"            traceback_message \u003d traceback.format_exc()"}],"source_content_type":"text/x-python","patch_set":10,"id":"fa6399be_bdcac06c","line":574,"range":{"start_line":569,"start_character":0,"end_line":574,"end_character":39},"in_reply_to":"fa6399be_ae09ba2e","updated":"2016-10-12 09:52:52.000000000","message":"the factory method can raise a TypeError, if a checker\u0027s contructor does not have the instances argument, which is caught here, at L569. The factory method is then called without the instances argument (defaulting to None), and the constructor is called without the instances argument.","commit_id":"444c64dc2534b618f3611aa6c1f94e27eb10a59c"}]}