Skip to content
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

Instrumenting build retrieval and unpacking times #4339

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

vitorguidi
Copy link
Collaborator

@vitorguidi vitorguidi commented Oct 18, 2024

Motivation

We currently lack metrics for build retrieval and unpacking times. This PR adds that, with granularity by fuzz target and job type.

There are two different implementations for build downloading/unpacking:

  • In the Build class, from which RegularBuild, SplitTargetBuild, FuchsiaBuild and SymbolizedBuild inherit the downloading/unpacking behavior
  • In the CustomBuild class, which implements its own logic

There are two possible cases for downloading/unpacking: clusterfuzz either downloads the whole build and unpacks it locally, or unpacks it remotely. This is the case for all build types except CustomBuild, which does not perform remote unpacking.

For build retrieval over http, we do not track download time. For all the other cases, it suffices to keep track of start/finish time for download and unpacking.

Part of #4271

Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than propagating this variable everywhere, can we just use the JOB_NAME env var? i.e.

job_name = environment.get_value('JOB_NAME')

This should be set in the uworkers at this point right @jonathanmetzman ?

@vitorguidi
Copy link
Collaborator Author

'JOB_NAME'

Rather than propagating this variable everywhere, can we just use the JOB_NAME env var? i.e.

job_name = environment.get_value('JOB_NAME')

This should be set in the uworkers at this point right @jonathanmetzman ?

That simplifies things a lot, thanks for the suggestion!

It seems like it does propagate to uworkers

uworker_env['JOB_NAME'] = environment.get_value('JOB_NAME')

@jonathanmetzman
Copy link
Collaborator

Rather than propagating this variable everywhere, can we just use the JOB_NAME env var? i.e.

job_name = environment.get_value('JOB_NAME')

This should be set in the uworkers at this point right @jonathanmetzman ?

Yes!

@jonathanmetzman
Copy link
Collaborator

'JOB_NAME'

Rather than propagating this variable everywhere, can we just use the JOB_NAME env var? i.e.

job_name = environment.get_value('JOB_NAME')

This should be set in the uworkers at this point right @jonathanmetzman ?

That simplifies things a lot, thanks for the suggestion!

It seems like it does propagate to uworkers

uworker_env['JOB_NAME'] = environment.get_value('JOB_NAME')

yep env vars definitely propagate to workers.

@vitorguidi vitorguidi force-pushed the feature/build-retrieval-metrics branch 2 times, most recently from faa97af to 621abbb Compare October 21, 2024 15:21
@vitorguidi vitorguidi force-pushed the feature/build-retrieval-metrics branch from 621abbb to eb8ae0c Compare October 21, 2024 15:25
Copy link
Collaborator

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

monitoring_metrics.JOB_BUILD_RETRIEVAL_TIME.add(
elapsed_time,
{
# The concept of a fuzz target does not apply
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this comment correct here? we are setting fuzz_target here.

build_download_time,
{
# The concept of a fuzz target does not apply
# to blackbox fuzzers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's more that custom builds don't support fuzz targets today, so a better comment would be:

"Custom builds don't support fuzz targets."

(We could probably fix this if needed -- I think we did support this in the past right @jonathanmetzman ?)

Copy link
Collaborator

@oliverchang oliverchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with some nits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants