From 73ac78f0725674c6cbcace48ff231234936f4456 Mon Sep 17 00:00:00 2001 From: Xavier-Do Date: Thu, 3 Jun 2021 10:31:58 +0000 Subject: [PATCH] [IMP] profiler: remove parameter 'path' Even if only accessible when writing some custom code, the path option of Profiler looked a little dangerous from a security point of view, since this option allows to write the result of the collector anywhere. This shouldn't be a problem, since Profiler shoudn't be accessible inside a safe_eval() and the path param cannot be user defined, but this is still a risk. An alternative option would have been to give a file descriptor instead, so that the user must have access to `open()`, which is unlikely inside a server action. This alternative has some disadvantages, though, because the file descriptor should be given when creating the profiler, or at least before `__exit__()`, removing the possibility to add the number of entries in the filename. This commit proposes another solution: add a generic way to output the profiler as a json text. The result can thus be saved the way the developer chooses. A utility method format_path() will help to format path the same way the profiler did before this change. --- odoo/tools/profiler.py | 62 ++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 24 deletions(-) diff --git a/odoo/tools/profiler.py b/odoo/tools/profiler.py index 57353f964e8..e616f91112d 100644 --- a/odoo/tools/profiler.py +++ b/odoo/tools/profiler.py @@ -5,7 +5,6 @@ import datetime import gc import json import logging -import os import sys import time import threading @@ -269,20 +268,18 @@ class Profiler: Context manager to use to start the recording of some execution. Will save sql and async stack trace by default. """ - def __init__(self, db=..., path=None, collectors=None, profile_session=None, + def __init__(self, collectors=None, db=..., profile_session=None, description=None, disable_gc=False, params=None): """ :param db: database name to use to save results. Will try to define database automatically by default. Use value ``None`` to not save results in a database. - :param path: path to use to save result :param collectors: list of string and Collector object Ex: ['sql', PeriodicCollector(interval=0.2)]. Use `None` for default collectors :param profile_session: session description to use to reproup multiple profile. use make_session(name) for default format. :param description: description of the current profiler Suggestion: (route name/test method/loading module, ...) :param disable_gc: flag to disable gc durring profiling (usefull to avoid gc while profiling, especially during sql execution) :param params: parameters usable by collectors (like frame interval) """ - self.path = path self.start_time = 0 self.duration = 0 self.profile_session = profile_session or make_session() @@ -297,7 +294,7 @@ class Profiler: if db is ...: # determine database from current thread db = getattr(threading.current_thread(), 'dbname', None) - if not db and not path: + if not db: # only raise if path is not given and db is not explicitely disabled raise Exception('Database name cannot be defined automaticaly. \n Please provide a valid/falsy dbname or path parameter') self.db = db @@ -337,25 +334,6 @@ class Profiler: collector.stop() self.duration = time.time() - self.start_time self._add_file_lines(self.init_stack_trace) - if self.path: - save_time = datetime.datetime.now().strftime("%Y%m%d-%H%M%S") - dirname = os.path.dirname(self.path) - os.makedirs(dirname, exist_ok=True) - for collector in self.collectors: - formatted_path = self.path.format(len=len(collector.entries), time=save_time) - row = collector.name - description = re.sub("[^0-9a-zA-Z-]+", "_", self.description) - path = f'{formatted_path}_{description}_{row}.json' - _logger.warning('saving to %s', path) - with open(path, 'w', encoding='utf-8') as f: - json.dump({ - "name": self.description, - "session": self.profile_session, - "create_date": save_time, - "init_stack_trace": _format_stack(self.init_stack_trace), - "duration": self.duration, - 'result': collector.entries, - }, f, indent=4) if self.db: # pylint: disable=import-outside-toplevel @@ -402,6 +380,42 @@ class Profiler: line = filelines[lineno - 1] stack[index] = (filename, lineno, name, line) + def entry_count(self): + """ Return the total number of entries collected in this profiler. """ + return sum(len(collector.entries) for collector in self.collectors) + + def format_path(self, path): + """ + Utility function to format a path for this profiler. + This is mainly useful to uniquify a path between executions. + """ + return path.format( + time=datetime.datetime.now().strftime("%Y%m%d-%H%M%S"), + len=self.entry_count(), + desc=re.sub("[^0-9a-zA-Z-]+", "_", self.description) + ) + + def json(self): + """ + Utility function to generate a json version of this profiler. + This is useful to write profiling entries into a file, such as:: + + with Profiler(db=None) as profiler: + do_stuff() + + filename = p.format_path('/home/foo/{desc}_{len}.json') + with open(filename, 'w') as f: + f.write(profiler.json()) + """ + return json.dumps({ + "name": self.description, + "session": self.profile_session, + "create_date": datetime.datetime.now().strftime("%Y%m%d-%H%M%S"), + "init_stack_trace": _format_stack(self.init_stack_trace), + "duration": self.duration, + "collectors": {collector.name: collector.entries for collector in self.collectors}, + }, indent=4) + class Nested: """