-
Notifications
You must be signed in to change notification settings - Fork 565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Monitoring] Adding fuzzing session duration metric #4336
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -1698,6 +1698,7 @@ def do_blackbox_fuzzing(self, fuzzer, fuzzer_directory, job_type): | |||
|
|||
def run(self): | |||
"""Run the fuzzing session.""" | |||
start_time = time.time() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with one note on this including fuzzer setup time, I wonder how much time this takes and whether it's more accurate to move the start time to line 1775 just before do_engine_fuzzing
and do_blackbox_fuzzing
are called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also add a comment before the FUZZING_SESSION_DURATION
definition to state that this includes fuzzer setup and data bundles update times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth measuring the setup time as well, otherwise we would be measuring fuzzing time (which is being tracked in #4356 ) + the time to upload metadata to datastore, which would be dominated by the fuzzing time, and probably make the metric redundant.
Adding the comment as requested
Motivation
We currently lack metrics for fuzzing session duration. This PR adds that as a histogram metric, with granularity by fuzzer, job and platform.
Part of #4271