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

feat(metric-stats): Report cardinality to metric stats #3360

Merged
merged 2 commits into from Apr 3, 2024

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Mar 28, 2024

Emits collected cardinality reports as metric stats.

Changed cardinality limit to a u32, this is still way too big for us ever to reach it, but it makes the conversion to f64 easier.

@Dav1dde Dav1dde requested a review from a team as a code owner March 28, 2024 16:03
@Dav1dde Dav1dde force-pushed the dav1d/feat/metric-stats-cardinality branch from d35eaa8 to 61221e9 Compare March 28, 2024 16:04
@Dav1dde Dav1dde self-assigned this Mar 28, 2024
@Dav1dde Dav1dde mentioned this pull request Mar 28, 2024
@Dav1dde Dav1dde force-pushed the dav1d/feat/metric-stats-cardinality branch from 61221e9 to 94cb3df Compare March 28, 2024 16:23
@@ -66,6 +77,35 @@ impl MetricStats {
.send(MergeBuckets::new(scoping.project_key, vec![volume]));
}

/// Tracks the cardinality of a metric.
pub fn cardinality(
Copy link
Member

Choose a reason for hiding this comment

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

Wdyt about track_cardinality? I find it more predictable from the API's user pov, even if it's similar to the track above.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@jjbayer sorry, I remember renaming it not sure what happened to it :(

relay-server/src/metric_stats.rs Show resolved Hide resolved
relay-server/src/metric_stats.rs Show resolved Hide resolved
relay-server/src/metric_stats.rs Show resolved Hide resolved
@@ -66,6 +77,35 @@ impl MetricStats {
.send(MergeBuckets::new(scoping.project_key, vec![volume]));
}

/// Tracks the cardinality of a metric.
pub fn cardinality(
Copy link
Member

Choose a reason for hiding this comment

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

relay-server/src/metric_stats.rs Show resolved Hide resolved
@Dav1dde Dav1dde enabled auto-merge (squash) April 3, 2024 08:36
@Dav1dde Dav1dde force-pushed the dav1d/feat/metric-stats-cardinality branch from c75d8ca to 239e000 Compare April 3, 2024 08:38
@Dav1dde Dav1dde merged commit ff241db into master Apr 3, 2024
20 checks passed
@Dav1dde Dav1dde deleted the dav1d/feat/metric-stats-cardinality branch April 3, 2024 09:04
jan-auer added a commit that referenced this pull request Apr 3, 2024
* master:
  feat(metric-stats): Report cardinality to metric stats (#3360)
  release: 0.8.56
  fix(perfscore): Adds span op tag to perf score totals (#3326)
  ref(profiles): Return retention_days as part of the Kafka message (#3362)
  ref(filter): Add GTmetrix to the list of web crawlers (#3363)
  fix: Fix kafka topic default (#3350)
  ref(normalization): Remove duplicated normalization (#3355)
  feat(feedback): Emit outcomes for user feedback events (#3026)
  feat(cardinality): Implement cardinality reporting (#3342)
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.

None yet

3 participants