Skip to content
Snippets Groups Projects

Log build dependencies cumulative size

Merged Grzegorz Bizon requested to merge fix/gb/instrument-build-dependencies-artifacts into master
All threads resolved!

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.

Edited by Grzegorz Bizon

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Grzegorz Bizon changed milestone to %15.3

    changed milestone to %15.3

  • 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

    added backend label

  • 6 Warnings
    :warning: This merge request includes more than 10 commits. Each commit should meet the following criteria:
    1. Have a well-written commit message.
    2. Has all tests passing when used on its own (e.g. when using git checkout SHA).
    3. Can be reverted on its own without also requiring the revert of commit that came before it.
    4. 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.

    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning: Please add a merge request subtype to this merge request.
    2 Messages
    :book: 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.

    :book: 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:

    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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

    • Resolved by 🤖 GitLab Bot 🤖

      :wave: @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

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 62600bb2 - Log build deps artifacts size when it is picked by a runner

    Compare with previous version

  • Grzegorz Bizon
  • Allure report

    allure-report-publisher generated test report!

    review-qa-blocking: :exclamation: test report for a425beca

    expand 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: :white_check_mark: test report for a425beca

    expand 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: :exclamation: test report for a425beca

    expand 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".

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 8c82d116 - Add specs for multiple dependencies' artifacts size logging

    Compare with previous version

  • Grzegorz Bizon marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

    marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

  • Grzegorz Bizon added 2 commits

    added 2 commits

    • 1162104a - Add size logger for different types of artifacts
    • 63b328b0 - Add build dependencies artifacts logger feature flag

    Compare with previous version

  • Grzegorz Bizon marked this merge request as ready

    marked this merge request as ready

  • A deleted user added feature flag label

    added feature flag label

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • d0c021dd - Add test checking if proper artifacts type is logged

    Compare with previous version

  • Grzegorz Bizon requested review from @furkanayhan

    requested review from @furkanayhan

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 35fdf19b - Improve specs for application context logger

    Compare with previous version

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 1d50e4fa - Regenerate GraphQL docs after updating context class

    Compare with previous version

  • A deleted user added documentation label

    added documentation label

  • Furkan Ayhan
  • Furkan Ayhan
  • Furkan Ayhan
  • Furkan Ayhan
  • Furkan Ayhan removed review request for @furkanayhan

    removed review request for @furkanayhan

  • Furkan Ayhan
  • Author Maintainer

    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.

  • reopened

  • Author Maintainer

    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 in json.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:

    1. Add json.meta.build_dependencies_size and json.meta.build_dependencies_count fields.
    2. Add json.meta.runner_type field.
    3. Do not change how json.meta.artifact_size is being calculated.
  • Grzegorz Bizon marked this merge request as draft

    marked this merge request as draft

  • Grzegorz Bizon changed title from Draft: Log build dependencies' artifacts size to Draft: Log build dependencies cumulative size

    changed title from Draft: Log build dependencies' artifacts size to Draft: Log build dependencies cumulative size

  • Grzegorz Bizon added 1125 commits

    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

    Compare with previous version

  • Grzegorz Bizon
  • Grzegorz Bizon added 2 commits

    added 2 commits

    • aa919041 - Update GraphQL reference documentation
    • f898ff51 - Update expected params in runner helper specs

    Compare with previous version

  • Grzegorz Bizon marked this merge request as ready

    marked this merge request as ready

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 1d25a651 - Fix application context logger argument name

    Compare with previous version

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 8859d3af - Log artifacts file size using artifacts file model

    Compare with previous version

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 7ec32646 - Add build dependencies logger rollout issue link

    Compare with previous version

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • ade7ec63 - Log build artifacts context upon API build request

    Compare with previous version

  • Grzegorz Bizon resolved all threads

    resolved all threads

  • Author Maintainer

    @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 Bizon
  • Grzegorz Bizon requested review from @stanhu

    requested review from @stanhu

  • Grzegorz Bizon
  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 8cfc5b6d - Allow logging artifacts size of other `artifactable` objects

    Compare with previous version

  • Stan Hu
  • Stan Hu
  • Stan Hu removed review request for @stanhu

    removed review request for @stanhu

  • Author Maintainer

    @stanhu I answered your questions and addressed suggestions! Can you please take a look again? Thanks!

  • Grzegorz Bizon requested review from @stanhu

    requested review from @stanhu

  • Grzegorz Bizon added 1 commit

    added 1 commit

    • 0d13d7c0 - Add more context to the job dependencies logger

    Compare with previous version

  • Stan Hu added 281 commits

    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

    Compare with previous version

  • Stan Hu added 220 commits

    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

    Compare with previous version

  • Stan Hu mentioned in merge request !93714 (merged)

    mentioned in merge request !93714 (merged)

  • Stan Hu added 37 commits

    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

    Compare with previous version

  • Stan Hu approved this merge request

    approved this merge request

  • Stan Hu resolved all threads

    resolved all threads

  • merged

  • Stan Hu mentioned in commit f11acfbe

    mentioned in commit f11acfbe

  • added workflowstaging label and removed workflowcanary label

  • Please register or sign in to reply
    Loading