Skip to content
Snippets Groups Projects

Add specs ensuring all metric definition key paths are present

What does this MR do?

Add specs ensuring all metric definition key paths are present in Usage Ping structure

Related to #322604 (closed).

This introduces a spec that checks if all key paths from metric definitions in config/metrics/**/*.yml files are present in actual Usage Ping data structure generated locally by Gitlab::UsageData and vice versa. It should act as a safeguard when adding and removing metrics and prevent unintended changes that would mutate the Usage Ping structure and are easily missed, like metrics that are dynamically generated from code.

At first, the test was failing, with a number of missing metrics or definitions (see #322604 (comment 553171773)) and after a thorough investigation, I've added metric definitions that, to my best knowledge, are genuinely missing.

Some key paths are omitted on purpose - because they are development fixtures (like mock_ci), yield data types that are more complex than a numerical value (like histogram or object types like topology metrics, which schema is not finalized yet). Some like user_auth_by_provider_* require further inspection.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Alper Akgun

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
  • 4 Warnings
    This merge request is quite big (625 lines changed), please consider splitting it into multiple merge requests.
    This MR has a Changelog file outside ee/, but code changes in ee/. Consider moving the Changelog file into ee/.
    10efbbc0: 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.

    For the following files, a review from the Data team and Product Intelligence team is recommended
    Please check the product intelligence guide.

    • lib/gitlab/usage_data.rb
    • lib/gitlab/usage_data_counters/known_events/analytics.yml
    • spec/lib/gitlab/usage_data_spec.rb
    • config/metrics/counts_28d/20210427102618_code_review_category_monthly_active_users.yml
    • config/metrics/counts_28d/20210427103010_code_review_extension_category_monthly_active_users.yml
    • config/metrics/counts_28d/20210427103119_code_review_group_monthly_active_users.yml
    • config/metrics/counts_28d/20210427105033_pipeline_authoring_total_unique_counts_monthly.yml
    • config/metrics/counts_28d/20210427213346_geo_secondary_web_oauth_users.yml
    • config/metrics/counts_7d/20210427103328_code_review_group_monthly_active_users.yml
    • config/metrics/counts_7d/20210427103407_code_review_category_monthly_active_users.yml
    • config/metrics/counts_7d/20210427103452_code_review_extension_category_monthly_active_users.yml
    • config/metrics/counts_7d/20210427105030_pipeline_authoring_total_unique_counts_weekly.yml
    • config/metrics/counts_all/20210423005644_i_analytics_dev_ops_adoption.yml
    • config/metrics/counts_all/20210427212450_geo_secondary_web_oauth_users.yml
    • config/metrics/counts_all/20210428142406_users_viewing_analytics_group_devops_adoption.yml
    • ee/config/metrics/counts_28d/20210405222005_i_incident_management_oncall_notification_sent_monthly.yml
    • ee/config/metrics/counts_28d/20210421080207_g_project_management_users_checking_epic_task_monthly.yml
    • ee/config/metrics/counts_28d/20210421102516_g_project_management_users_unchecking_epic_task_monthly.yml
    • ee/config/metrics/counts_28d/20210507165054_custom_compliance_frameworks.yml
    • ee/config/metrics/counts_28d/20210507171840_epic_boards_usage_total_unique_counts_monthly.yml
    • ee/config/metrics/counts_7d/20210405220139_i_incident_management_oncall_notification_sent_weekly.yml
    • ee/config/metrics/counts_7d/20210421075943_g_project_management_users_checking_epic_task_weekly.yml
    • ee/config/metrics/counts_7d/20210421102812_g_project_management_users_unchecking_epic_task_weekly.yml
    • ee/config/metrics/counts_7d/20210507171838_epic_boards_usage_total_unique_counts_weekly.yml
    • doc/development/usage_ping/dictionary.md
    2 Messages
    📖 We are in the process of rolling out a new workflow for adding changelog entries. This new workflow uses Git commit subjects and Git trailers to generate changelogs. This new approach will soon replace the current YAML based approach.

    To ease the transition process, we recommend you start using both the old and new approach in parallel. This is not required at this time, but will make it easier to transition to the new approach in the future. To do so, pick the commit that should go in the changelog and add a Changelog trailer to it. For example:

    This is my commit's subject line
    
    This is the optional commit body.
    
    Changelog: added

    The value of the Changelog trailer should be one of the following: added, fixed, changed, deprecated, removed, security, performance, other.

    For more information, take a look at the following resources:

    If you'd like to see the new approach in action, take a look at the commits in the Omnibus repository.

    📖 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/development/usage_ping/dictionary.md

    The review does not need to block merging this merge request. See the:

    Reviewer roulette

    Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

    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.

    Category Reviewer Maintainer
    backend Tiger Watson (@tigerwnz) (UTC+12, 10 hours ahead of @pskorupa) Rémy Coutable (@rymai) (UTC+2, same timezone as @pskorupa)
    product intelligence Luis Mejia (@lmejia2) (UTC-6, 8 hours behind @pskorupa) Maintainer review is optional for product intelligence

    If needed, you can retry the danger-review job that generated this comment.

    Generated by 🚫 Danger

    Edited by 🤖 GitLab Bot 🤖
  • Piotr Skorupa added 883 commits

    added 883 commits

    • cdae95c7...05c4148a - 880 commits from branch master
    • bb44119f - Add specs checking metric definitions against Usage Ping key paths
    • 7ce36310 - Add stubbing Prometheus and omit removed metric definitions
    • 952e23fe - Add email and company to License factory licensee

    Compare with previous version

  • Piotr Skorupa added 1 commit

    added 1 commit

    • 9b05d2df - Remove dev service names from UsageData on local

    Compare with previous version

  • mentioned in issue #27954 (closed)

  • changed milestone to %13.12

  • Piotr Skorupa added 930 commits

    added 930 commits

    • 9b05d2df...ec5edc5f - 926 commits from branch master
    • 58527607 - Add specs checking metric definitions against Usage Ping key paths
    • 12162410 - Add stubbing Prometheus and omit removed metric definitions
    • 6f8fc73a - Add email and company to License factory licensee
    • 4b112941 - Remove dev service names from UsageData on local

    Compare with previous version

  • Piotr Skorupa added 2 commits

    added 2 commits

    • e4bb9c2f - Add ignoring keys for definitions specs
    • e646c383 - Fix incident management metric definition key paths

    Compare with previous version

  • Piotr Skorupa added 4 commits

    added 4 commits

    • 380be9b0 - Update metric definitions spec ignored metrics
    • 92bcd878 - Refactor metric definitions spec
    • a0137da6 - Remove custom RSpec message length config setting
    • 17e0559d - Add missing metric definitions

    Compare with previous version

  • Piotr Skorupa added 545 commits

    added 545 commits

    • 17e0559d...5781efdb - 535 commits from branch master
    • 7d99feeb - Add specs checking metric definitions against Usage Ping key paths
    • 6ae46b2d - Add stubbing Prometheus and omit removed metric definitions
    • 219fa252 - Add email and company to License factory licensee
    • 31f8bc57 - Remove dev service names from UsageData on local
    • e406279d - Add ignoring keys for definitions specs
    • 13a588a0 - Fix incident management metric definition key paths
    • fd679149 - Update metric definitions spec ignored metrics
    • d6eac519 - Refactor metric definitions spec
    • ef7b399c - Remove custom RSpec message length config setting
    • f2222fa5 - Add missing metric definitions

    Compare with previous version

  • Piotr Skorupa added 1 commit

    added 1 commit

    • 9ad27b18 - Move consts in describe block to a memoized let

    Compare with previous version

  • Piotr Skorupa added 2 commits

    added 2 commits

    Compare with previous version

  • Piotr Skorupa marked this merge request as ready

    marked this merge request as ready

  • Piotr Skorupa marked the checklist item I have included a changelog entry. as completed

    marked the checklist item I have included a changelog entry. as completed

  • Piotr Skorupa added 2 commits

    added 2 commits

    • ac8712e4 - Fix cleaning up Gitlab::Usage::MetricDefinitions specs
    • 61abe4cc - Refactor metric definition spec

    Compare with previous version

  • 🤖 GitLab Bot 🤖 added 1 deleted label

    added 1 deleted label

  • Piotr Skorupa added 332 commits

    added 332 commits

    • 61abe4cc...4296b032 - 321 commits from branch master
    • 8a4c69c5 - - Add specs checking metric definitions against Usage Ping key paths
    • 17824dc1 - Fix incident management metric definition key paths
    • 054d4b89 - Update metric definitions spec ignored metrics
    • 05cc64ff - Refactor metric definitions spec
    • ff0e84a2 - Remove custom RSpec message length config setting
    • d5bc8e85 - Add missing metric definitions
    • 1d351576 - Move consts in describe block to a memoized let
    • 844c3318 - Add changelog entry
    • d3d1dbc5 - Update Metrics Dictionary
    • 4baefe48 - Fix cleaning up Gitlab::Usage::MetricDefinitions specs
    • 43de7996 - Refactor metric definition spec

    Compare with previous version

  • Piotr Skorupa removed 1 deleted label

    removed 1 deleted label

  • Piotr Skorupa resolved all threads

    resolved all threads

  • Piotr Skorupa added 13 commits

    added 13 commits

    • 23cd5954 - Add specs checking metric definitions against Usage Ping key paths
    • 3610f6d3 - Fix incident management metric definition key paths
    • 58792cb7 - Update metric definitions spec ignored metrics
    • 01860f51 - Refactor metric definitions spec
    • 45e1381a - Remove custom RSpec message length config setting
    • 0f095fbe - Add missing metric definitions
    • 734a8ffe - Move consts in describe block to a memoized let
    • aca75ad4 - Add changelog entry
    • b4342d42 - Add missing analytics events metric definition
    • 61a05e4f - Update Metrics Dictionary
    • b135e34f - Fix cleaning up Gitlab::Usage::MetricDefinitions specs
    • b0069a79 - Refactor metric definition spec
    • 82562e7c - Add stubbing UsageData db counts in metrics spec

    Compare with previous version

  • 🤖 GitLab Bot 🤖 added 1 deleted label

    added 1 deleted label

  • Piotr Skorupa added 1 commit

    added 1 commit

    • 3cf5eea7 - Add stubbing UsageData db counts in metrics spec

    Compare with previous version

  • Piotr Skorupa changed the description

    changed the description

  • Piotr Skorupa removed 1 deleted label

    removed 1 deleted label

  • Piotr Skorupa requested review from @drew and @a_akgun

    requested review from @drew and @a_akgun

  • Alper Akgun
  • Alper Akgun
  • Alper Akgun
  • Alper Akgun removed review request for @a_akgun

    removed review request for @a_akgun

  • Alper Akgun
  • drew stachon
  • drew stachon
  • drew stachon
  • drew stachon
  • drew stachon approved this merge request

    approved this merge request

  • Alper Akgun approved this merge request

    approved this merge request

  • Author Maintainer

    @rspeicher Could you do a backend maintainer pass? 🙇

    I'm planning to rebase and partially squash this in a moment, so don't mind the >10 commits Danger warning.

    Edited by Piotr Skorupa
  • Piotr Skorupa requested review from @rspeicher

    requested review from @rspeicher

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading