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
-
📋 Does this MR need a changelog?-
I have included a changelog entry. -
I have not included a changelog entry because _____.
-
-
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
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
Merge request reports
Activity
changed milestone to %13.11
added 1 commit
- cdae95c7 - Add specs checking metric definitions against Usage Ping key paths
- Resolved by Piotr Skorupa
added backend label
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 inee/
. Consider moving the Changelog file intoee/
.⚠ 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:
https://gitlab.com/gitlab-com/gl-infra/delivery/-/issues/1564
- https://docs.gitlab.com/ee/api/repositories.html#generate-changelog-data
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:
-
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! 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
🚫 DangerEdited by 🤖 GitLab Bot 🤖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
Toggle commit list-
cdae95c7...05c4148a - 880 commits from branch
added 1 commit
- 9b05d2df - Remove dev service names from UsageData on local
added product intelligence product intelligencereview pending labels and removed analytics instrumentationonboarding issue label
mentioned in issue #27954 (closed)
changed milestone to %13.12
added missed:13.11 label
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
Toggle commit list-
9b05d2df...ec5edc5f - 926 commits from branch
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
Toggle commit list-
17e0559d...5781efdb - 535 commits from branch
added 1 commit
- 9ad27b18 - Move consts in describe block to a memoized let
added 2 commits
added documentation label
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
Toggle commit list-
61abe4cc...4296b032 - 321 commits from branch
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
Toggle commit listadded 1 commit
- 3cf5eea7 - Add stubbing UsageData db counts in metrics spec
- Resolved by Robert Speicher
- Resolved by Robert Speicher
@a_akgun Could you give it a pass for product intelligence?
🙏
- Resolved by Robert Speicher
- Resolved by Alper Akgun
- Resolved by Alper Akgun
removed review request for @a_akgun
- Resolved by Alper Akgun
- Resolved by drew stachon
- Resolved by drew stachon
- Resolved by drew stachon
- Resolved by drew stachon
added product intelligenceapproved label and removed product intelligencereview pending label
@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 Skoruparequested review from @rspeicher