Service Ping review guidelines

This page includes introductory material for a Analytics Instrumentation review, and is specific to Service Ping related reviews. For broader advice and general best practices for code reviews, refer to our code review guide.

Resources for reviewers

• Service Ping Guide
• Metrics Dictionary

Review process

We recommend a Analytics Instrumentation review when a merge request (MR) touches any of the following Service Ping files:

• usage_data* files.
• The Metrics Dictionary, including files in:
  • config/metrics.
  • ee/config/metrics.
  • schema.json.
• Analytics Instrumentation tooling. For example, Gitlab::UsageMetricDefinitionGenerator

Roles and process

The merge request author should

• Decide whether a Analytics Instrumentation review is needed. You can skip the Analytics Instrumentation review and remove the labels if the changes are not related to the Analytics Instrumentation domain and are regular backend changes.
• If a Analytics Instrumentation review is needed, add the labels ~analytics instrumentation and ~analytics instrumentation::review pending.
• For merge requests authored by Analytics Instrumentation team members:
  • Assign both the ~backend and ~analytics instrumentation reviews to another Analytics Instrumentation team member.
  • Assign the maintainer review to someone outside of the Analytics Instrumentation group.
• Assign an engineer from the Analytics Instrumentation team for a review.
• Set the correct attributes in the metric's YAML definition:
  • product_section, product_stage, product_group
  • Provide a clear description of the metric.
• Add a changelog according to guidelines.

The Analytics Instrumentation reviewer should

• Perform a first-pass review on the merge request and suggest improvements to the author.
• Check the metric's location in the Service Ping JSON payload.
• Add the ~database label and ask for a database review for metrics that are based on Database.
• Add ~Data Warehouse::Impact Check for any database metric that has a query change. Changes in queries can affect data operations.
• For tracking using Redis HLL (HyperLogLog):
  • Check if a feature flag is needed.
• For a metric's YAML definition:
  • Check the metric's description.
  • Check the metric's key_path.
  • Check the product_section, product_stage, and product_group fields. Read the stages file.
  • Check the file location. Consider the time frame, and if the file should be under ee.
  • Check the tiers.
• If a metric was changed or removed: Make sure the MR author notified the Customer Success Ops team (@csops-team), Analytics Engineers (@gitlab-data/analytics-engineers), and Product Analysts (@gitlab-data/product-analysts) by @ mentioning those groups in a comment on the issue for the MR and all of these groups have acknowledged the removal.
• Make sure that the new metric is available in Service Ping payload, by running: Gitlab::Usage::ServicePingReport.for(output: :all_metrics_values).dig(*'key_path'.split('.')) with key_path substituted by the new metric's key_path.
• Metrics instrumentations
  • Recommend using metrics instrumentation for new metrics, if possible.
• Approve the MR, and relabel the MR with ~"analytics instrumentation::approved".

Review workload distribution

Danger bot adds the list of changed Analytics Instrumentation files and pings the @gitlab-org/analytics-section/analytics-instrumentation/engineers group for merge requests that are not drafts.

Any of the Analytics Instrumentation engineers can be assigned for the Analytics Instrumentation review.