Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(metrics): Use MRI for metrics extraction [INGEST-939] #1215

Merged
merged 7 commits into from
Apr 7, 2022

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented Mar 29, 2022

Introduces Metric Resource Identifiers (MRI) for metrics extraction from sessions and transactions.

Background

MRIs follow three core principles:

  1. Robustness: Metrics must be addressed via a stable identifier. During ingestion in Relay and Snuba, metrics are preaggregated and bucketed based on this identifier, so it cannot change over time without breaking bucketing.
  2. Uniqueness: The identifier for metrics must be unique across variations of units and metric types, within and across use cases, as well as between projects and organizations.
  3. Abstraction: The user-facing product changes its terminology over time, and splits concepts into smaller parts. The internal metric identifiers must abstract from that, and offer sufficient granularity to allow for such changes.

Schema

MRIs have the format <type>:<ns>/<name>@<unit>, comprising the following components:

  • Type: counter (c), set (s), distribution (d), gauge (g), and evaluated (e) for derived numeric metrics.
  • Namespace: Identifying the product entity and use case affiliation of the metric.
  • Name: The display name of the metric in the allowed character set.
  • Unit: The verbatim unit name.

Note that the / character could cause troubles when not properly escaped in URL path parameters. Ingestion will mostly place the MRIs in query strings and POST bodies, where this should not be an issue. The user-facing APIs do not expose MRIs.

Namespaces

Namespaces allow to identify the product entity that the metric got extracted from, and/or identify the use case that the metric belongs to. These namespaces cannot be defined freely, instead they are defined by Sentry. Over time, there should be more namespaces.

Some namespaces can be considered internal. These would not be allowed on the public ingestion endpoints, and would not be exposed via the user-facing Metrics API by default. Relay obtains a form of ACL (access control list) that determines which namespaces can be ingested.

The initial namespaces are:

  • Performance monitoring (public): transactions
  • Errors (public): errors
  • Issues (internal): issues
  • Release Health (public): sessions
  • Alerting (internal): alerts
  • Custom user-defined metrics (public): custom

Next Steps

  • Update the allow list in Sentry. See also fix(metrics): Add 'ops.' to breakdown metric names sentry#33044
  • Update the aggregation protocol and rely on the unit in the metric name. Also, introduce a helper to parse and validate the namespace in the metric name.
  • Normalize and sanitize metric names and tag names, replacing invalid characters.
  • Rename units and define units for all extracted metrics.

@jan-auer jan-auer changed the title ref(metrics): Use MRI for metrics extraction ref(metrics): Use MRI for metrics extraction [INGEST-939] Mar 29, 2022
@jan-auer jan-auer marked this pull request as ready for review March 30, 2022 07:32
@jan-auer jan-auer requested a review from a team March 30, 2022 07:32
@jan-auer jan-auer self-assigned this Mar 30, 2022
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not merge until we have the naming layer in sentry in place.

Copy link
Member

@untitaker untitaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is totally fine, but I would like to see the PR for sentry done in parallel. I feel like there's some nasty surprises waiting there (particularly around the testharness)

@jan-auer jan-auer enabled auto-merge (squash) April 7, 2022 15:38
* master:
  fix: Apply clippy 1.60 lints (#1220)
  build: Update symbolic to 8.7.0 (#1216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants