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): Refactor aggregation error, recover from errors more gracefully [ingest-1204] #1240

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

untitaker
Copy link
Member

See discussion in #1235

This PR introduces variants to aggregation error, and skips over broken buckets instead of dropping the entire batch.

@untitaker untitaker requested a review from a team April 25, 2022 11:52
@@ -716,8 +716,26 @@ impl Bucket {

/// Any error that may occur during aggregation.
#[derive(Debug, Fail)]
#[fail(display = "failed to aggregate metrics")]
pub struct AggregateMetricsError;
#[fail(display = "failed to aggregate metrics: {}", kind)]
Copy link
Member

Choose a reason for hiding this comment

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

nit: not sure how this composes outside, but you might be able to go without the prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do without prefix but then I need to put the prefix into the log message, otherwise the sentry errors just print "invalid character" without context

@untitaker untitaker merged commit 85f49cc into master Apr 25, 2022
@untitaker untitaker deleted the ref/aggregation-error branch April 25, 2022 12:22
@untitaker untitaker changed the title ref(metrics): Refactor aggregation error, recover from errors more gracefully ref(metrics): Refactor aggregation error, recover from errors more gracefully [ingest-1204] Apr 25, 2022
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.
Instructions and example for changelog

For changes exposed to the Python package, please add an entry to py/CHANGELOG.md. This includes, but is not limited to event normalization, PII scrubbing, and the protocol.

For changes to the Relay server, please add an entry to CHANGELOG.md under the following heading:

  1. Features: For new user-visible functionality.
  2. Bug Fixes: For user-visible bug fixes.
  3. Internal: For features and bug fixes in internal operation, especially processing mode.

To the changelog entry, please add a link to this PR (consider a more descriptive message):

- Refactor aggregation error, recover from errors more gracefully. ([#1240](https://github.com/getsentry/relay/pull/1240))

If none of the above apply, you can opt out by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 3ef65b3

jan-auer added a commit that referenced this pull request Apr 27, 2022
* master:
  fix: Add bucket key to extra (#1243)
  fix(metrics): Remove/reject nul-bytes from metric strings (#1235)
  chore: Bump sentry-release-parser (#1242)
  fix(metrics): Fix MRI validation in statsd protocol (#1241)
  ref(metrics): Refactor aggregation error, recover from errors more gracefully (#1240)
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.

2 participants