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): Aggregate metrics before rate limiting #3746

Merged
merged 7 commits into from
Jun 24, 2024
Merged

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Jun 19, 2024

Implements the rest of #3662.
Fixes #3553.

Removes all pre-aggregation and rate limit steps before we receive a project state, all rate limiting is done after aggregation.

@Dav1dde Dav1dde changed the title ref(metrics): Aggregate before rate limiting ref(metrics): Aggregate metrics before rate limiting Jun 19, 2024
@Dav1dde Dav1dde self-assigned this Jun 19, 2024
@Dav1dde Dav1dde marked this pull request as ready for review June 19, 2024 14:16
@Dav1dde Dav1dde requested a review from a team as a code owner June 19, 2024 14:16
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Q: what's the behavior if the project config isn't fetched, do we keep aggregating metrics indefinitely?

relay-server/src/services/project_cache.rs Outdated Show resolved Hide resolved
relay-server/src/services/project.rs Outdated Show resolved Hide resolved
relay-server/src/services/project.rs Show resolved Hide resolved
@Dav1dde
Copy link
Member Author

Dav1dde commented Jun 20, 2024

Q: what's the behavior if the project config isn't fetched, do we keep aggregating metrics indefinitely?

Yes.

Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

LGTM.

In the future, there should be a limit on the number of metrics Relay accepts if a project config can't be fetched.

CHANGELOG.md Show resolved Hide resolved
relay-server/src/services/project.rs Show resolved Hide resolved
relay-server/src/services/processor.rs Outdated Show resolved Hide resolved
relay-server/src/services/project.rs Outdated Show resolved Hide resolved
@Dav1dde Dav1dde enabled auto-merge (squash) June 24, 2024 10:03
@Dav1dde Dav1dde disabled auto-merge June 24, 2024 10:03
@Dav1dde Dav1dde enabled auto-merge (squash) June 24, 2024 10:04
@Dav1dde Dav1dde merged commit 4ff3d83 into master Jun 24, 2024
23 checks passed
@Dav1dde Dav1dde deleted the dav1d/agg-early branch June 24, 2024 10:08
jan-auer added a commit that referenced this pull request Jun 27, 2024
* master:
  chore(dynamic-sampling): Remove metrics for dsc tracking (#3766)
  feat(web-vitals): add support for mobile browsers (#3762)
  feat(profiles): Support profiler_id in context (#3714)
  ref(normalization): Add origin and event_type tags to normalization decision (#3764)
  feat(rate-limiting): Add back docs with examples on rate limiting (#3761)
  feat(spans): Correctly emit negative outcomes for rate limited transactions that have nested spans (#3749)
  ref(metrics): Remove unused sentry extra data (#3758)
  feat(statsd): Emit tokio runtime metrics via statsd (#3755)
  ref(metrics): Aggregate metrics before rate limiting (#3746)
  ref(cogs): Remove unused metric, revert to released usage accountant (#3756)
  build(cargo): Update curve25519-dalek from 4.0.0 to 4.1.3 (#3745)
  test(deps): Bump requests from 2.31.0 to 2.32.2 (#3752)
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.

ERROR relay_server::services::project
3 participants