Log build dependencies cumulative size
What does this MR do and why?
This merge request adds additional instrumentation that will allow us to log the size of all build dependencies' artifacts exposed when a runner picks a job to process.
See gitlab-org/gitlab#362897
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
changed milestone to %15.3
added grouppipeline security label
assigned to @grzesiek
Suggested Reviewers (beta)
The individuals below may be good candidates to participate in the review based on various factors.
You can use slash commands in comments to quickly assign
/assign_reviewer @user1
.Suggested Reviewers @rspeicher
,@mayra-cabrera
,@psimyn
,@rymai
,@jivanvl
If you do not believe these suggestions are useful, please apply the label Bad Suggested Reviewer. You can also provide feedback for this feature on this issue:
https://gitlab.com/gitlab-org/gitlab/-/issues/357923
.Automatically generated by Suggested Reviewers Bot - an experimental ML-based recommendation engine created by ~"group::applied ml".
Edited by GitLab Reviewer-Recommender Bot- A deleted user
added backend label
6 Warnings This merge request includes more than 10 commits. Each commit should meet the following criteria: - Have a well-written commit message.
- Has all tests passing when used on its own (e.g. when using git checkout SHA).
- Can be reverted on its own without also requiring the revert of commit that came before it.
- Is small enough that it can be reviewed in isolation in under 30 minutes or so.
If this merge request contains commits that do not meet this criteria and/or contains intermediate work, please rebase these commits into a smaller number of commits or split this merge request into multiple smaller merge requests.
080471bd: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 0a5571e8: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 31ed374f: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 22951702: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. Please add a merge request subtype to this merge request. 2 Messages CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, add the
Changelog
trailer to the commit message you want to add to the changelog.If you want to create a changelog entry for GitLab EE, also add the
EE: true
trailer to your commit message.If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/api/graphql/reference/index.md
The review does not need to block merging this merge request. See the:
-
Metadata for the
*.md
files that you've changed. The first few lines of each*.md
file identify the stage and group most closely associated with your docs change. - The Technical Writer assigned for that stage and group.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Sashi Kumar Kumaresan ( @sashi_kumar
) (UTC+5.5, 3.5 hours ahead of@grzesiek
)Heinrich Lee Yu ( @engwan
) (UTC+8, 6 hours ahead of@grzesiek
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- Resolved by 🤖 GitLab Bot 🤖
@grzesiek - please add typebug typefeature, typemaintenance or a subtype label to this merge request.- typebug: Defects in shipped code and fixes for those defects. This includes all the bug types (availability, performance, security vulnerability, mobile, etc.)
- typefeature: Effort to deliver new features, feature changes & improvements. This includes all changes as part of new product requirements like application limits.
- typemaintenance: Up-keeping efforts & catch-up corrective improvements that are not Features nor Bugs. This includes restructuring for long-term maintainability, stability, reducing technical debt, improving the contributor experience, or upgrading dependencies.
See the handbook for more guidance on classifying.
This message was created with automation and Engineering Productivity is looking for feedback in this issue:
https://gitlab.com/gitlab-org/quality/engineering-productivity/team/-/issues/43
added 1 commit
- 62600bb2 - Log build deps artifacts size when it is picked by a runner
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
Allure report
allure-report-publisher
generated test report!review-qa-blocking:
test report for a425becaexpand test summary
+-----------------------------------------------------------------------------------------+ | suites summary | +------------------------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Plan | 47 | 0 | 1 | 47 | 48 | ❗ | | Create | 28 | 0 | 1 | 28 | 29 | ❗ | | Manage | 34 | 0 | 2 | 36 | 36 | ❗ | | Verify | 14 | 0 | 1 | 14 | 15 | ❗ | | Protect | 2 | 0 | 0 | 2 | 2 | ❗ | | Secure | 2 | 0 | 0 | 2 | 2 | ❗ | | Configure | 0 | 0 | 1 | 0 | 1 | ➖ | | Feature flag handler sanity checks | 9 | 0 | 0 | 9 | 9 | ❗ | | Version sanity check | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------------------------+--------+--------+---------+-------+-------+--------+ | Total | 136 | 0 | 7 | 138 | 143 | ❗ | +------------------------------------+--------+--------+---------+-------+-------+--------+
package-and-qa-ff-enabled:
test report for a425becaexpand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Fulfillment | 6 | 0 | 33 | 0 | 39 | ✅ | | Plan | 171 | 0 | 0 | 0 | 171 | ✅ | | Create | 471 | 0 | 18 | 0 | 489 | ✅ | | Secure | 63 | 0 | 6 | 0 | 69 | ✅ | | Manage | 300 | 0 | 9 | 0 | 309 | ✅ | | Configure | 0 | 0 | 9 | 0 | 9 | ➖ | | Verify | 141 | 0 | 9 | 0 | 150 | ✅ | | Package | 0 | 0 | 9 | 0 | 9 | ➖ | | Release | 15 | 0 | 0 | 0 | 15 | ✅ | | Analytics | 6 | 0 | 0 | 0 | 6 | ✅ | | Version sanity check | 0 | 0 | 3 | 0 | 3 | ➖ | | Protect | 6 | 0 | 0 | 0 | 6 | ✅ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 1179 | 0 | 96 | 0 | 1275 | ✅ | +----------------------+--------+--------+---------+-------+-------+--------+
package-and-qa-ff-disabled:
test report for a425becaexpand test summary
+---------------------------------------------------------------------------+ | suites summary | +----------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +----------------------+--------+--------+---------+-------+-------+--------+ | Create | 471 | 0 | 18 | 0 | 489 | ✅ | | Secure | 63 | 0 | 6 | 3 | 69 | ❗ | | Manage | 300 | 0 | 9 | 6 | 309 | ❗ | | Plan | 171 | 0 | 0 | 0 | 171 | ✅ | | Fulfillment | 6 | 0 | 33 | 0 | 39 | ✅ | | Verify | 141 | 0 | 9 | 0 | 150 | ✅ | | Version sanity check | 0 | 0 | 3 | 0 | 3 | ➖ | | Configure | 0 | 0 | 9 | 0 | 9 | ➖ | | Release | 15 | 0 | 0 | 0 | 15 | ✅ | | Analytics | 6 | 0 | 0 | 0 | 6 | ✅ | | Package | 0 | 0 | 9 | 0 | 9 | ➖ | | Protect | 6 | 0 | 0 | 0 | 6 | ✅ | +----------------------+--------+--------+---------+-------+-------+--------+ | Total | 1179 | 0 | 96 | 9 | 1275 | ❗ | +----------------------+--------+--------+---------+-------+-------+--------+
Setting label(s) devopsverify sectionops based on ~"group::pipeline insights".
added devopsverify sectionops labels
added 1 commit
- 8c82d116 - Add specs for multiple dependencies' artifacts size logging
marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed
- A deleted user
added feature flag label
added 1 commit
- d0c021dd - Add test checking if proper artifacts type is logged
added typemaintenance label
- Resolved by Furkan Ayhan
@furkanayhan can you please review this merge request? Thanks in advance!
Edited by Grzegorz Bizon
requested review from @furkanayhan
- The
gitlab-qa-mirror
downstream pipeline for !93179 (d0c021dd) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (d0c021dd) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (35fdf19b) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (35fdf19b) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (1d50e4fa) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (1d50e4fa) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (f5db71a6) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (f5db71a6) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (7ec32646) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (7ec32646) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (ade7ec63) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (ade7ec63) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (8cfc5b6d) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (8cfc5b6d) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (0d13d7c0) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (0d13d7c0) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (a6aab83b) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (a6aab83b) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (0af80648) failed! - The
gitlab-qa-mirror
downstream pipeline for !93179 (0af80648) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (a425beca) passed. - The
gitlab-qa-mirror
downstream pipeline for !93179 (a425beca) passed.
- The
added 1 commit
- 35fdf19b - Improve specs for application context logger
added 1 commit
- 1d50e4fa - Regenerate GraphQL docs after updating context class
- A deleted user
added documentation label
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Furkan Ayhan
- Resolved by Grzegorz Bizon
- Resolved by Furkan Ayhan
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
- Resolved by Grzegorz Bizon
removed review request for @furkanayhan
- Resolved by Grzegorz Bizon
Thanks for the review @furkanayhan! I answered to your questions, will address your remark shortly!
- Resolved by Stan Hu
We had an interesting discussion with @tmaczukin about this problem today. We discovered that this merge request will not improve our instrumentation, because the build dependencies' artifacts are already instrumented through a different endpoint. Instead of doing that we will improve user-agents in Runner / Runner artifacts-download-helper / Workhorse. I'm going to close this merge request.
I reopened this merge request as it seems that we still need to instrument
/api/v4/jobs/request
endpoint, but we don't want to log them injson.meta.artifact_size
. Instead we want to have a different field, so that it does not break our existing metrics, that now have been fixed: https://gitlab.com/gitlab-org/gitlab/-/issues/360462#note_1047770253.What we should do here now:
- Add
json.meta.build_dependencies_size
andjson.meta.build_dependencies_count
fields. - Add
json.meta.runner_type
field. - Do not change how
json.meta.artifact_size
is being calculated.
- Add
- Resolved by Grzegorz Bizon
added 1125 commits
-
1d50e4fa...18ab40f3 - 1116 commits from branch
master
- 3caab3e1 - Refactor logging artifacts size to reduce duplication
- a9ab6f36 - Log build deps artifacts size when it is picked by a runner
- 62274d49 - Add specs for multiple dependencies' artifacts size logging
- af8c70b7 - Add size logger for different types of artifacts
- 330d8f66 - Add build dependencies artifacts logger feature flag
- ad7daea1 - Add test checking if proper artifacts type is logged
- e02ba3d5 - Improve specs for application context logger
- ec3e4e8b - Regenerate GraphQL docs after updating context class
- f5db71a6 - Extract build dependencies log to a separate field
Toggle commit list-
1d50e4fa...18ab40f3 - 1116 commits from branch
- Resolved by Grzegorz Bizon
added 1 commit
- 1d25a651 - Fix application context logger argument name
added 1 commit
- 8859d3af - Log artifacts file size using artifacts file model
added 1 commit
- 7ec32646 - Add build dependencies logger rollout issue link
added 1 commit
- ade7ec63 - Log build artifacts context upon API build request
@stanhu would you be able to give this MR an early review? I'm taking a few days off until Monday, so no rush with that, but I will try to address your feedback as soon as possible so that we could perhaps merge it on Monday.
Edited by Grzegorz Bizonrequested review from @stanhu
- Resolved by Grzegorz Bizon
added 1 commit
- 8cfc5b6d - Allow logging artifacts size of other `artifactable` objects
- Resolved by Stan Hu
- Resolved by Grzegorz Bizon
removed review request for @stanhu
added pipeline:skip-undercoverage label
@stanhu I answered your questions and addressed suggestions! Can you please take a look again? Thanks!
requested review from @stanhu
added 1 commit
- 0d13d7c0 - Add more context to the job dependencies logger
- Resolved by Stan Hu
The Review App is failing here. I'll try to rebase to see if it's unrelated.
added 281 commits
-
0d13d7c0...802a6dd2 - 264 commits from branch
master
- 12489c4c - Refactor logging artifacts size to reduce duplication
- 952c9de4 - Log build deps artifacts size when it is picked by a runner
- eb4d8ab0 - Add specs for multiple dependencies' artifacts size logging
- be63e19e - Add size logger for different types of artifacts
- 6511ec52 - Add build dependencies artifacts logger feature flag
- 66646790 - Add test checking if proper artifacts type is logged
- 91c44b23 - Improve specs for application context logger
- ec925a2d - Regenerate GraphQL docs after updating context class
- e85bfb19 - Extract build dependencies log to a separate field
- 98a72504 - Update GraphQL reference documentation
- c35e0872 - Update expected params in runner helper specs
- 461bd14f - Fix application context logger argument name
- 04ad0c56 - Log artifacts file size using artifacts file model
- 37d8445d - Add build dependencies logger rollout issue link
- aa3f11bb - Log build artifacts context upon API build request
- 9e518877 - Allow logging artifacts size of other `artifactable` objects
- a6aab83b - Add more context to the job dependencies logger
Toggle commit list-
0d13d7c0...802a6dd2 - 264 commits from branch
added 220 commits
-
a6aab83b...efb62c0d - 203 commits from branch
master
- fd3a70c3 - Refactor logging artifacts size to reduce duplication
- e805b85c - Log build deps artifacts size when it is picked by a runner
- 48eea67a - Add specs for multiple dependencies' artifacts size logging
- b99ab07e - Add size logger for different types of artifacts
- c39de50e - Add build dependencies artifacts logger feature flag
- e32f717a - Add test checking if proper artifacts type is logged
- 240de75e - Improve specs for application context logger
- ff3c2658 - Regenerate GraphQL docs after updating context class
- 304a659e - Extract build dependencies log to a separate field
- 5f071199 - Update GraphQL reference documentation
- 1eb40cc5 - Update expected params in runner helper specs
- af98b2a2 - Fix application context logger argument name
- 75287a4a - Log artifacts file size using artifacts file model
- 558d944d - Add build dependencies logger rollout issue link
- 144fbf8a - Log build artifacts context upon API build request
- cd666a6a - Allow logging artifacts size of other `artifactable` objects
- 0af80648 - Add more context to the job dependencies logger
Toggle commit list-
a6aab83b...efb62c0d - 203 commits from branch
mentioned in merge request !93714 (merged)
added 37 commits
-
0af80648...21c3d652 - 19 commits from branch
master
- 9f09f6a1 - Refactor logging artifacts size to reduce duplication
- 22951702 - Log build deps artifacts size when it is picked by a runner
- a244d505 - Add specs for multiple dependencies' artifacts size logging
- 31ed374f - Add size logger for different types of artifacts
- e76dcf59 - Add build dependencies artifacts logger feature flag
- b2453080 - Add test checking if proper artifacts type is logged
- 00d2c198 - Improve specs for application context logger
- 51d14471 - Regenerate GraphQL docs after updating context class
- 0a5571e8 - Extract build dependencies log to a separate field
- 2e824dcd - Update GraphQL reference documentation
- cc0379d5 - Update expected params in runner helper specs
- e04688e5 - Fix application context logger argument name
- 6cc6bc71 - Log artifacts file size using artifacts file model
- 465ed730 - Add build dependencies logger rollout issue link
- be311dac - Log build artifacts context upon API build request
- aa0e712c - Allow logging artifacts size of other `artifactable` objects
- 080471bd - Add more context to the job dependencies logger
- a425beca - Fix Rubocop test failure
Toggle commit list-
0af80648...21c3d652 - 19 commits from branch
mentioned in commit f11acfbe
added workflowstaging-canary label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
mentioned in issue gitlab-org/quality/triage-reports#20589 (closed)
mentioned in issue gitlab-org/quality/triage-reports#20978 (closed)
mentioned in issue gitlab-org/quality/triage-reports#21525 (closed)
mentioned in issue gitlab-org/quality/triage-reports#22029