ENH: Add migas telemetry in addition to sentry #2817
Conversation
mgxd
requested review from
oesteban and
effigies
and removed request for
oesteban
.circleci/config.yml
Outdated
| @@ -794,7 +794,7 @@ jobs: | |||
| --fs-no-reconall --use-syn-sdc --ignore slicetiming \ | |||
| --dummy-scans 1 --sloppy --write-graph \ | |||
| --output-spaces MNI152NLin2009cAsym \ | |||
| --mem-mb 14336 --nthreads 4 -vv | |||
| --mem-mb 14336 --nthreads 4 -vv --notrack | |||
There was a problem hiding this comment.
How much effort would it be to split debug/production pings for migas? It seems useful that we cross-compare with sentry on these debug heartbeats.
fmriprep/cli/run.py
Outdated
| @@ -43,6 +44,7 @@ def main(): | |||
| from ..utils.sentry import sentry_setup | |||
|
|
|||
| sentry_setup() | |||
| ping_migas() | |||
There was a problem hiding this comment.
Is in this first ping when migas will check the user and figure out whether it is cached?
If not, I would favor to replicate sentry's design and have a migas_setup() call here.
I would imagine one day we will feed arguments here to help apps customize the settings:
migas_setup(userid="<some identifier generated by the app>", appid="override/fmriprep")
fmriprep/cli/run.py
Outdated
| @@ -71,13 +73,15 @@ def main(): | |||
| config.load(config_file) | |||
|
|
|||
| if config.execution.reports_only: | |||
| ping_migas(status="success" if retcode == 0 else "error") | |||
There was a problem hiding this comment.
May we abide by OS' standards and just:
ping_migas(status=retcode)Then let migas do the mapping.
fmriprep/cli/run.py
Outdated
| sys.exit(int(retcode > 0)) | ||
|
|
||
| if fmriprep_wf and config.execution.write_graph: | ||
| fmriprep_wf.write_graph(graph2use="colored", format="svg", simple_form=True) | ||
|
|
||
| retcode = retcode or (fmriprep_wf is None) * EX_SOFTWARE | ||
| if retcode != 0: | ||
| ping_migas(status="error") |
There was a problem hiding this comment.
What about replacing all these ping_migas with status with an exit hook? - https://docs.python.org/3/library/atexit.html
Since the hook does not allow to access the exit code, we probably want to:
- Add a new
statusorexitcodeconfig setting (perhaps under theexecutionsection). Initialize it at -1. - Define some codes: e.g., 1001 (I haven't really thought this through, this might not be a good code) is set right before building the workflow, then 1002 is set after it is built and before calling
run()and finally 0 is set when it is complete and successful. An error in the pipeline overrides the exitcode too. - The
atexit()hook will not send a ping if the code is still -1, and otherwise, submit whatever code it has.
fmriprep/utils/sentry.py
Outdated
| if config.execution.notrack: | ||
| return | ||
|
|
||
| os.environ['ENABLE_MIGAS'] = 'yes' |
There was a problem hiding this comment.
| os.environ['ENABLE_MIGAS'] = 'yes' |
fmriprep/utils/sentry.py
Outdated
|
|
||
| import migas | ||
|
|
||
| if config.execution.notrack: |
There was a problem hiding this comment.
| if config.execution.notrack: | |
| if ( | |
| config.execution.notrack | |
| or os.getenv("MIGAS_DISABLE", "no").lower() in ("yes", "on", "1", "true") | |
| ): |
fmriprep/utils/sentry.py
Outdated
| """Communicate with the migas telemetry server.""" | ||
| import os | ||
|
|
||
| import migas |
There was a problem hiding this comment.
delay after the following if branch?
fmriprep/utils/sentry.py
Outdated
| session_id = config.execution.run_uuid.split('_')[1] | ||
|
|
||
| # TODO: check for bad versions | ||
| res = migas.add_project( |
There was a problem hiding this comment.
this looks like what migas_setup() would look like inside.
- Add MIGAS_OPTOUT to let the client filter out pings - Add `--notrack` to let fmriprep filter out pings - Allow final ds210 fmriprep run to ping server and be caught/classified as CI
Co-authored-by: Chris Markiewicz <[email protected]>
This adds
migastelemetry in addition to sentry, to allow us to compare +/-'s