G-UTILS-GPROFILER REWORK (2): Rework complex files #261
Conversation
granulatedekel
changed the base branch from
master
to
add-unedited-gprofiler-files-in
granulatedekel
force-pushed
the
refactor-gprofiler-utils-in
branch
from
c092236 to
40b789f
Compare
granulatedekel
changed the title
granulatedekel
changed the title
granulatedekel
added
the
g-utils-gprofiler-rework
| @@ -247,6 +244,7 @@ def _kill_and_reap_process(process: Popen, kill_signal: signal.Signals) -> Tuple | |||
|
|
|||
| def run_process( | |||
| cmd: Union[str, List[str]], | |||
| logger: Union[logging.LoggerAdapter, logging.Logger], | |||
There was a problem hiding this comment.
is there a reason we don’t want to enforce this as keyword argument?
|
I'm changing the source branch to |
| return wrapper | ||
|
|
||
|
|
||
| def start_process( |
There was a problem hiding this comment.
start_process, run_process et al are very coupled to gProfiler at several points:
- the logging of command + output
- staticx
- removal of LD_LIBRARY_PATH
and add some optional features that we want for gProfiler but not for other callers: - pdeathsig
Let's keep it in gProfiler, alongside all relevant logic:
get_staticx_dirstart_processrun_processset_child_termination_on_parent_deathprctl- (and any other function not used in this PR after the removal of that function).
| os.unlink(f) | ||
|
|
||
|
|
||
| def wait_for_file_by_prefix( |
There was a problem hiding this comment.
Unused? Move back to gProfiler.
| return Path(output_files[0]) | ||
|
|
||
|
|
||
| def reap_process(process: Popen) -> Tuple[int, bytes, bytes]: |
| return result | ||
|
|
||
|
|
||
| def pgrep_maps(match: str, logger: Union[logging.LoggerAdapter, logging.Logger]) -> List[Process]: |
| _INSTALLED_PROGRAMS_CACHE: List[str] = [] | ||
|
|
||
|
|
||
| def assert_program_installed(program: str) -> None: | ||
| if program in _INSTALLED_PROGRAMS_CACHE: | ||
| return | ||
|
|
||
| if shutil.which(program) is not None: | ||
| _INSTALLED_PROGRAMS_CACHE.append(program) | ||
| else: | ||
| raise ProgramMissingException(program) |
| ) | ||
|
|
||
|
|
||
| class JattachJcmdRunner: |
There was a problem hiding this comment.
Convert this to accept a run_process callable, gProfiler will use its run_process function and other components will pass their own wrappers.
The type of the callable should be defined in granulate-utils.
| self._libap_path_glibc = libap_path_glibc | ||
| self._libap_path_musl = libap_path_musl | ||
|
|
||
| # assert mode in ("cpu", "itimer", "alloc"), f"unexpected mode: {mode}" |
There was a problem hiding this comment.
Need to bring this assert back (potentially with other event types you wish to support)
| def _run_async_profiler(self, cmd: List[str]) -> str: | ||
| try: | ||
| # kill jattach with SIGTERM if it hangs. it will go down | ||
| run_process( |
There was a problem hiding this comment.
As commented for JattachJcmdRunner, please have a parameter for this class to control the callable to run processes with. When instantiating JattachJcmdRunner, you'll pass the same value from this class.
There was a problem hiding this comment.
These can be moved back to gProfiler
There was a problem hiding this comment.
Most of these can be moved back to gProfiler
Part of the G-UTILS-GPROFILER REWORK saga - which aims to move away shared code from gprofiler to the g-utils project:
#263 #261 intel/gprofiler#925 intel/gprofiler#926 intel/gprofiler#926
This ultimate PR (for this repo) is responsible for logical changes and "trimming" of complex files to be untied from the gprofiler platform