Metrics utilization #25
Conversation
This PR contains the implementation of a decorator to collect metrics from DeepVariant Runner. A second PR will utilize the newly defined decorator to collect usage metrics.
We define a decorator to utilize metrics module and collect usage metrics from deepvariant_runner.
|
Please note this PR is the continuation of #19 meaning that metrics.py and metrics_test.py is branched from that PR. |
| @@ -1040,10 +1102,57 @@ def run(argv=None): | |||
| 'jobs. By default, the pipeline runs all 3 jobs (make_examples, ' | |||
| 'call_variants, postprocess_variants) in sequence. ' | |||
| 'This option may be used to run parts of the pipeline.')) | |||
| parser.add_argument( | |||
| '--stop_collecting_anonymous_usage_metrics', | |||
There was a problem hiding this comment.
I think add the argument collecting_anonymous_usage_metrics would be easier to read. Especially it is used as if not pipeline_args.stop_collecting_anonymous_usage_metrics:. And the action and help seem not relevant?
There was a problem hiding this comment.
I agree the if statement would be more readable if this flag was not 'negative'. However, I really like this flag to explicitly indicate it stops metrics collection.
Please let me know if this does not make sense.
|
|
||
| if not pipeline_args.stop_collecting_anonymous_usage_metrics: | ||
| metrics.add( | ||
| _get_project_number(pipeline_args.project), |
There was a problem hiding this comment.
I am not sure whether it still makes sense to collect the metrics when getting the project number failed (-1).
There was a problem hiding this comment.
Yes it does. Ideally we like to know how many unique users we have and getting their project_id is needed for that. But in case we cannot get the project_id still it will be informative if we find out other information about the run (how many workers, failure or success, ...).
There was a problem hiding this comment.
Do you know in which case it may fail to get the project_id?
There was a problem hiding this comment.
I can imagine there might be some GCP IAM settings that prevents a service account from reading the project's metadata, I am guessing in those cases it will also raise an error.
It is also possible we can successfully fetch the project_id in all runs and never return -1. But in any case in _get_project_number() we have to run the gcloud projects describe command in a try-catch block.
There was a problem hiding this comment.
According to gcloud projects describe --help:
This command can fail for the following reasons:
- The project specified does not exist.
- The active account does not have permission to access the given project
The first case does not occur to us (otherwise the gcloud alpha genomics pipelines run would't go through). So we will not log the right project_id when serviceaccount does not have enough permission.
There was a problem hiding this comment.
In general we aren't allowed to log project ID (it's PII). I think we got approval to log a SHA1 hash of the project ID.
Note that the permissions required to get the project ID are non-trivial and so it might be likely that the SA doesn't have them.
There was a problem hiding this comment.
Thank you Saman for the detailed explanation! I think the second case should not occur to us neither, since the user is running the pipeline in that project, which they must have access to. So I agree that it probably will never return -1 :).
There was a problem hiding this comment.
Sorry I misread this as ID not number for some reason. Project number is fine :)
gcp_deepvariant_runner.py
Outdated
| if pipeline_args.stop_collecting_anonymous_usage_metrics: | ||
| func(pipeline_args, *args, **kwargs) | ||
| else: | ||
| success = False |
There was a problem hiding this comment.
super nit: How about status = '_Failure', and after the func succeeds, having status = '_Success', so you can avoid if else below.
samanvp
force-pushed
the
metrics_utilization
branch
from
0a677df to
f05ca90
Compare
gcp_deepvariant_runner.py
Outdated
| if pipeline_args.stop_collecting_anonymous_usage_metrics: | ||
| func(pipeline_args, *args, **kwargs) | ||
| else: | ||
| success = False |
| @@ -1040,10 +1102,57 @@ def run(argv=None): | |||
| 'jobs. By default, the pipeline runs all 3 jobs (make_examples, ' | |||
| 'call_variants, postprocess_variants) in sequence. ' | |||
| 'This option may be used to run parts of the pipeline.')) | |||
| parser.add_argument( | |||
| '--stop_collecting_anonymous_usage_metrics', | |||
There was a problem hiding this comment.
I agree the if statement would be more readable if this flag was not 'negative'. However, I really like this flag to explicitly indicate it stops metrics collection.
Please let me know if this does not make sense.
|
|
||
| if not pipeline_args.stop_collecting_anonymous_usage_metrics: | ||
| metrics.add( | ||
| _get_project_number(pipeline_args.project), |
There was a problem hiding this comment.
Yes it does. Ideally we like to know how many unique users we have and getting their project_id is needed for that. But in case we cannot get the project_id still it will be informative if we find out other information about the run (how many workers, failure or success, ...).
We define a decorator to utilize metrics module and collect anonymous usage
metrics from deepvariant_runner.