Adds workflow_job_seconds metric #5
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Intention to support instantiation of server for integration testing. Side effect is improving readability. * Extracts root handler into func. * Error logging handled by main.
* Intention is to test against package interface using server_test package. Couldn't get to work with main_test. * Side effect is improved readability due to less global state. * ghActionsExporter private to package as created in Server setup
* Intention was to improve coverage of http server implementation. * Probably makes more sense to test the handlers directly for most cases.
* Backfills tests before behaviour changes. * CheckRun test only checks for 200 due to async behaviour.
Assumes job queue time can be calculated from difference between job start time and start time of first step. Labels: * runner_group to identify queues for specific runners * state queued as durations can also be published for completed jobs * no workflow label as it is not possible to identify workflow without additional API calls
* Simplifies testing by moving observation of workflow job metrics under interface. * Leaves other metrics untouched for now, even though inconsistent.
* Use custom serve mux to allow for creation of multiple servers in integration tests.
|
looks great, i will run locally, let me know when you are ready and then set the PR out from draft! |
|
Awesome, thanks! The build failed - the lint stage passes locally for me. The error suggests something about updating the tool so I've bumped the golangci-lint version to latest: |
This is possible as it seems step started_at times are rounded to the closest second whereas job started_at time has miliseconds included.
|
I think this is ready for review. I've tested the exporter locally and ran a couple of workflows. I spotted a few issues which I've fixed up and added tests for. |
janakerman
force-pushed
the
jan-workflow-job-metrics
branch
from
a8eab39
to
5528cde
Compare
cpanato
approved these changes
Apr 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #4
This PR brings a number of improvements, mainly to support testing:
workflow_job_duration_secondsworkflow_jobeventsstatewhich has two possible values:queued- the amount of time the job spent queued waiting for a runnerin_progress- the amount of time the job spent in progress whilst completing all stepsWorkflowJobObserverinterface created to allow for testing of observations without interacting with Prometheus lib.Serverstruct to allow for start + stop of server with different configurations.The PR brings more changes than actually needed to add the new metric (i.e extracting behaviour to a
Serverstruct in an additional package. I can try to unpick these changes into a smaller but I think the other changes are worth considering.The individual commits should largely be complete and documented.
Still TODO as part of this PR:
Remove check_run event handling- this doesn't make sense - we'd need to add the workflow event handler for that one and I'd rather do that as a separate PR.Recordworkflow_execution_time_secondsusing theworkflow_jobcompletedeventStill TODO hopefully out of scope for this PR: