)]}'
{"monasca_agent/collector/checks_d/process.py":[{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"28f7299de554ed1549677b933e89417e89e677d6","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                    found_process_list \u003d [proc.pid for proc in"},{"line_number":51,"context_line":"                                          self._current_process_list]"},{"line_number":52,"context_line":"                    break"},{"line_number":53,"context_line":"                if exact_match:"},{"line_number":54,"context_line":"                    found_process_list \u003d \\"},{"line_number":55,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":56,"context_line":"                         if proc.name \u003d\u003d string]"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_666d5f32","line":53,"updated":"2016-03-07 10:25:22.000000000","message":"minor nit; if you use \u0027elif\u0027 here then you don\u0027t need the preceding \u0027break\u0027","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"28f7299de554ed1549677b933e89417e89e677d6","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_0649fb8b","line":60,"updated":"2016-03-07 10:25:22.000000000","message":"I believe that this will run the \u0027 \u0027.join(...) once for every item in self._current_process_list, which isn\u0027t very efficient.\n\ne.g;\n\n    def f(): print(\"hello\"); return (1,2,3)\n    [x for x in range(10) if x in f()]","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"0a9b4144b520602897ef79fcf63df04864225d61","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_dead068a","line":60,"in_reply_to":"5aef4532_0649fb8b","updated":"2016-03-07 16:39:17.000000000","message":"My understanding of this is that we\u0027re looking for partial matches across the whole command line so we can\u0027t look for just a single list element.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"5b3380892182d733306e54dca87f03e6ea7d6352","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_19e9481b","line":60,"in_reply_to":"5aef4532_192668ba","updated":"2016-03-07 17:21:45.000000000","message":"Ah, sorry, I didn\u0027t realise that partial matches are a use-case!\n\nGiven that, the only way that comes to mind to speed this up would be to cache the joined strings (changing the cmdline after process start is fairly rare...) but that may be more trouble than it\u0027s worth at this point.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"55decb930a747c3eb55adafcc533244f51971323","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_ea2597c2","line":60,"in_reply_to":"5aef4532_19e9481b","updated":"2016-03-07 18:00:01.000000000","message":"That\u0027s a good idea.  It should be pretty easy to do in this review.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"15d71616019d50c41ed253d2a56b9b5abf5a6395","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_35824495","line":60,"in_reply_to":"5aef4532_55655043","updated":"2016-03-07 18:15:20.000000000","message":"We rebuild the cache each collection cycle so we should be ok for PID reuse.  The changing command line case we could try in our test environments and see how well it works but if that is a normal use case that sounds like it could throw off our monitoring either way.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"10f99f87320ccfbeda1deae6ac00d1f1000f94a7","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_192668ba","line":60,"in_reply_to":"5aef4532_dead068a","updated":"2016-03-07 16:39:57.000000000","message":"You are correct that it is expensive though.  I think it\u0027s currently the second or third most expensive part of this check.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"29fd12e12b50c8cfbb4ab03f9047af09c08d9946","unresolved":false,"context_lines":[{"line_number":57,"context_line":"                else:"},{"line_number":58,"context_line":"                    found_process_list \u003d \\"},{"line_number":59,"context_line":"                        [proc.pid for proc in self._current_process_list"},{"line_number":60,"context_line":"                         if string in \u0027 \u0027.join(proc.cmdline)]"},{"line_number":61,"context_line":"        return set(found_process_list)"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"    def get_process_metrics(self, pids, psutil, name):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_55655043","line":60,"in_reply_to":"5aef4532_ea2597c2","updated":"2016-03-07 18:12:19.000000000","message":"Well, there are gotchas like cmdlines changing (apparently mysqld does that to mask passwords) and expiring entries on PID reuse...","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"28f7299de554ed1549677b933e89417e89e677d6","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                self._current_process_list.append(p)"},{"line_number":156,"context_line":"            except psutil.NoSuchProcess:"},{"line_number":157,"context_line":"                # No way to log useful information here so just move on"},{"line_number":158,"context_line":"                pass"},{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_c93ba49b","line":158,"updated":"2016-03-07 10:25:22.000000000","message":"With the move of exception handlers to the top level, a single NoSuchProcess will now result in no data being gathered, but no log messages to explain why - I think it may be better to log something in this case?","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"0a9b4144b520602897ef79fcf63df04864225d61","unresolved":false,"context_lines":[{"line_number":155,"context_line":"                self._current_process_list.append(p)"},{"line_number":156,"context_line":"            except psutil.NoSuchProcess:"},{"line_number":157,"context_line":"                # No way to log useful information here so just move on"},{"line_number":158,"context_line":"                pass"},{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_de4ea6c0","line":158,"in_reply_to":"5aef4532_c93ba49b","updated":"2016-03-07 16:39:17.000000000","message":"This should still allow the for to continue and process the rest of the iterator, right?\n\nWe don\u0027t want to log here because NoSuchProcess occurs any time we walk across a process that has gone away and since the process_iter() call looks at all the processes on the system we get lots of spurious log messages about transient processes.\n\nFurther down, in the check method, we test the number of  PIDs we found for a given process and we will generate a measurement there if the process we\u0027re looking for is missing.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"28f7299de554ed1549677b933e89417e89e677d6","unresolved":false,"context_lines":[{"line_number":156,"context_line":"            except psutil.NoSuchProcess:"},{"line_number":157,"context_line":"                # No way to log useful information here so just move on"},{"line_number":158,"context_line":"                pass"},{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"},{"line_number":162,"context_line":"                self.log.info(\u0027Error: %s\u0027 % e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_e99ae0c4","line":159,"updated":"2016-03-07 10:25:22.000000000","message":"By moving the exception handler to the top level this will now fail to gather any data if there\u0027s a subset of processes for which permission is denied, while before it would have gathered at least some data. Is that the intention?","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"5b3380892182d733306e54dca87f03e6ea7d6352","unresolved":false,"context_lines":[{"line_number":156,"context_line":"            except psutil.NoSuchProcess:"},{"line_number":157,"context_line":"                # No way to log useful information here so just move on"},{"line_number":158,"context_line":"                pass"},{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"},{"line_number":162,"context_line":"                self.log.info(\u0027Error: %s\u0027 % e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_04bc3c04","line":159,"in_reply_to":"5aef4532_de2fc69f","updated":"2016-03-07 17:21:45.000000000","message":"Sorry, I managed to misread the code - you\u0027re right that this isn\u0027t a problem.\n\nOn the AccessDenied question - this is because in Linux prior to 3.3 all users could read all /proc/*/cmdline. In 3.3 kernels onwards you can mount /proc to prevent this (see hidepid option in http://man7.org/linux/man-pages/man5/proc.5.html) however apparently most distributions don\u0027t use that at the moment.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":14273,"name":"Joe Keen","email":"joe.keen@hpe.com","username":"jkeen"},"change_message_id":"0a9b4144b520602897ef79fcf63df04864225d61","unresolved":false,"context_lines":[{"line_number":156,"context_line":"            except psutil.NoSuchProcess:"},{"line_number":157,"context_line":"                # No way to log useful information here so just move on"},{"line_number":158,"context_line":"                pass"},{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"},{"line_number":162,"context_line":"                self.log.info(\u0027Error: %s\u0027 % e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_de2fc69f","line":159,"in_reply_to":"5aef4532_e99ae0c4","updated":"2016-03-07 16:39:17.000000000","message":"Previously we were caching the list of processes and then processing them in a lower for loop.  There we generated exceptions but they were always exceptions based on the original cache. All we\u0027ve done is move the exceptions up to the point where we build the cache so we don\u0027t have to check again later.\n\nThis should do the same iteration that was done before and rather than detecting a process in the cache that can\u0027t be accessed simply not add an inaccessible process to the cache.\n\nDo you think this will prevent building a viable cache?\n\nAs a side note we also don\u0027t understand why we never see this message.  I would have thought there were always at least some processes on the system we couldn\u0027t access since this code isn\u0027t running as root but that doesn\u0027t appear to be the case.","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"},{"author":{"_account_id":15799,"name":"Ben Motz","email":"ben.launchpad@ouat.co.uk","username":"bmotz"},"change_message_id":"28f7299de554ed1549677b933e89417e89e677d6","unresolved":false,"context_lines":[{"line_number":159,"context_line":"            except psutil.AccessDenied as e:"},{"line_number":160,"context_line":"                self.log.info(\u0027Access denied to process {0}\u0027.format("},{"line_number":161,"context_line":"                    process.name()))"},{"line_number":162,"context_line":"                self.log.info(\u0027Error: %s\u0027 % e)"},{"line_number":163,"context_line":""},{"line_number":164,"context_line":"    def check(self, instance):"},{"line_number":165,"context_line":"        try:"}],"source_content_type":"text/x-python","patch_set":3,"id":"5aef4532_2909682d","line":162,"updated":"2016-03-07 10:25:22.000000000","message":"It\u0027s better to make a single log entry; in log aggregation systems this will result in two separate entries: Log entries of the form \"INFO: Error: foo\" can be misleading! (And if you\u0027re searching for \"Error\" in your log entries then you\u0027ll miss the preceding line that explains the context)","commit_id":"3467a3fb222cfecaf5af25823d910667e7df65f1"}]}