Repository navigation
Add corpus synchronization time metric - #5530
brun0-matheus wants to merge 11 commits into
Conversation
Add a metric do keep track of corpus synchronization. The metric also distinguishes between corpus download/upload and whether it was successful or not.
Add unittest to check if SYNC_CORPUS_TIME is being emmited correctly. It tests all 4 possible combinations of (`success`, `is_upload`).
|
Hi, I think it would be nice to make it clear why the metric is only for engine fuzzing. |
decoNR
left a comment
There was a problem hiding this comment.
Just to confirm, were you able to verify that these were the only two functions associated with corpus sync?
It's because blackbox fuzzing does not use corpus. |
There is one function missing: |
Creates a context manager `TimeMetricTracker` to automatically track time. It supports Counter and CumulativeDistribution metrics. `_MockMetric` and `stub_unavailable` were modified to include the metric type that is being mocked, in order for the tracker to correctly determine the metric type during testing. Also, the `is_upload` label was removed from the SYNC_CORPUS_TIME metric.
During preprocessing, there are some possibly time intensive operations regarding listing corpus files and/or signing URLs. Added code to track time spent in this region. Also added unittests to check this.
nit: I think it does use but it does not sync corpus. |
decoNR
left a comment
There was a problem hiding this comment.
Please update the description to match your recent refactors.
I like to add a section of "Key changes" with the reason for each one (e.g https://depot.code.corp.goog/gosst/casper/pull/408), feel free to do the same if you think it is good.
| max_upload_urls=_get_max_corpus_uploads_per_task(), | ||
| max_download_urls=25000, | ||
| use_backup=True).serialize()) | ||
| with _track_sync_corpus_metric(fuzz_target.fully_qualified_name()): |
There was a problem hiding this comment.
I think we only want to track in main. Can you investigate how this copy differs from the corpus sync?
There was a problem hiding this comment.
This is a setup for future corpus sync operations. It lists files on the corpus and signs URLs for download/upload, which can be time consuming. So I believe it's a good idea to track this, since it's related to corpus sync.
There was a problem hiding this comment.
Nice, agreed.
Before closing the associated bug, document all of these decisions (the final state) there so we can easily revisit it in the future.
There was a problem hiding this comment.
Maybe should use another metric for this. IMO the corpus sync metric is meant to determine how much time we spent syncing corpus before running the fuzz session in main. Adding this here will cause confusion afterwards once we start trying to analyze each fuzz step duration.
We could later also try to have metrics for preprocess (like how much time we spend signing URLs) and postprocess.
There was a problem hiding this comment.
I believe that adding a 'stage' label addresses this issue, and also allows us to have more granularity (since in the main stage there are two places with corpus sync: on startup to download the corpus and once it finishes, to send new corpus files).
If in the future we decide to use another metric for this, we can just change the metric and it will be done.
I talked to @PauloVLB, and he confirmed that it doesn't use corpus. It uses something different called data bundle. |
|
Hey @decoNR and @brun0-matheus. Just trying to clarify:
Yeah, there's some confusion here because of naming, but mostly yes: blackbox fuzzing doesn't use a corpus the way engine fuzzing does.
So keeping this metric engine only makes sense to me. Data bundle sync is a separate path in setup, so it's out of scope here. |
Nice! Thanks for the details. What I meant is that, in a way, it still uses a corpus; the only difference is how it is stored and synced. Stating that "blackbox does not use a corpus" makes it sound as it relies on a completely different mechanism. |
Add to the description that the setup time is also measured in the metric.
Create a generic method `insert` in Metric, which automatically handles which method to call and does integer conversition if needed. Also change TimeMetricTracker and the correspondings tests.
ViniciustCosta
left a comment
There was a problem hiding this comment.
fuzzer: [...] My conclusion is that the fully_qualified_name (which is the fuzz_task fully_qualified_name for engine fuzzing) fully describes the corpus, so there is no need to keep a job label.
The fact that the fully qualified name describes the corpus is unrelated to the metric label itself. We will use the metric label to understand if certain jobs or fuzzers take more time than other, for example. Multiple jobs can have the same fully qualified fuzzer name.
One thing to notice is that, for engine fuzzers, fuzzer_name is the fuzz engine, while for blackbox fuzzers, it is the specific fuzzer (since there is no fuzz target).
AFAICT there are two approaches for current labels in cf:
- The
FUZZING_SESSION_DURATIONapproach, which uses a CumulativeDistribution metric type with fuzzer_name (not the fully quilified name that includes the fuzz target to avoid high cardinality) and job. - The
FUZZER_TOTAL_FUZZ_TIME/JOB_TOTAL_FUZZ_TIMEapproach, which uses a Counter metric with separate metrics for per job and per fuzz target visibility.
It comes down to answering "is the goal of the metric system/job-level observability or to compare with TOTAL_FUZZ_TIME and pinpoint slow individual fuzz_targets? We can discuss offline to decide that
| metric: Metric, | ||
| labels=None, | ||
| has_success_label: bool = False): | ||
| # If |has_success_label| is true, a "success" label will be added upon exit, |
There was a problem hiding this comment.
Add this to the docstring (go/pystyle#class-docs)
The stage label can have 3 values: preprocess, download and upload.
Create a SplitMetricHelper to aid emission of this type of metric.
As discussed offline, I decided to do the |
Bug: b/568557979
Overview
Creates new metrics called
FUZZER_SYNC_CORPUS_TIMEandJOB_SYNC_CORPUS_TIMEto keep track of the time spent doing corpus synchronization in engine fuzzing (since blackbox fuzzing does not use corpus). The metric has 4 labels, and this is the rationale I used to choose them:fuzzer: (only used inFUZZER_SYNC_CORPUS_TIME) the fuzzer name, like inFUZZER_TOTAL_FUZZ_TIME.job: (only used inJOB_SYNC_CORPUS_TIME) the job name, like inJOB_TOTAL_FUZZ_TIME.platform: I use it because many other similar metrics use it as well (likeTOTAL_FUZZ_TIME).runtime: Same reason as above.stage: The stage of corpus synchronization. Can be either 'preprocess', 'upload' or 'download'.success: Self-explanatory.Changes
FUZZER_SYNC_CORPUS_TIMEandJOB_SYNC_CORPUS_TIMEcreated, which are being tracked infuzz_task.py.TimeMetricTrackerinmetrics.monitor, which is a context manager to automatically track time. Works for any metric except Gauge.insertmethod in theMetricclass. This method receives a float value and calls the correct method to add this value to the metric (increment_byfor Counter,addfor Distribution andsetfor Gauge). It also converts float to int if the metric only supports int.SplitMetricHelperinmonitor.py. This helper allows to emit FUZZER_* and JOB_* with a single function call. CurrentlyTests
Unit tests
TrackSyncCorpusTimeTestcreated infuzz_task_test.py, which tests if the metric is being measured correctly for success and failure cases.Unit tests
TimeMetricTrackerTestcreated inmonitor_test.py.I deployed in the dev environment and it's working as intended. Evidence in the bug comments.