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

fix(profiles): Enforce profile quotas on metrics buckets #2212

Merged
merged 4 commits into from
Jun 13, 2023

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Jun 13, 2023

In #2163 we decided not to enforce rate limits on processed profiles with limit > 0, to prevent complexity (see PR description).

I wrongly assumed that quotas (i.e. rate limits with limit:0) would be correctly enforced by dropping the profile envelope item before metrics are extracted. I overlooked the fact that PoP Relays (where metrics are extracted) do not receive quota configuration, because they do not request it:

full_config: config.processing_enabled(),

Note that this only affects rate limits that are issued only for profiles. Rate limits issued for transactions correctly drop the processed profiles.

This PR fixes the issue by stripping the has_profile tag off metrics buckets if a Profile rate limit is active.

Fixes #2207

#skip-changelog

@jjbayer jjbayer changed the title Fix/profile remove tags fix(profiles): Enforce profile quotas on metrics buckets Jun 13, 2023
@jjbayer jjbayer marked this pull request as ready for review June 13, 2023 11:18
@jjbayer jjbayer requested a review from a team June 13, 2023 11:18
@jjbayer jjbayer merged commit 94c2a3a into master Jun 13, 2023
20 checks passed
@jjbayer jjbayer deleted the fix/profile-remove-tags branch June 13, 2023 13:48
jan-auer added a commit that referenced this pull request Jun 14, 2023
* master: (32 commits)
  feat(txname): Mark all URL transactions as sanitized (#2210)
  fix(profiles): Enforce profile quotas on metrics buckets (#2212)
  revert(spans): Move span metrics back to transactions namespace (#2206)
  instr(metrics): Add values to invalid metric errors (#2201)
  ref(spans): Move span metrics to spans namespace (#2205)
  chore(protocol): Add missing py/changelog entry for lock property (#2203)
  build(py): Move pytest.ini to tests directory (#2204)
  ref: Use serde collect_str to avoid allocations (#2149)
  chore(gocd): Auto approve deploy relay-pop on check success (#2200)
  test(document-pii): Fix flaky test (#2198)
  feat(spans): Extract transaction `user` into span's databag (#2197)
  feat(spans): Extract `release` tag from the transaction to the span (#2189)
  ref(metrics-extraction): Split span and transaction metric extraction (#2191)
  ref(crons): Support GET in the check-in endpoint (#2192)
  ref(spans): Introduce opinionated casing in span metrics (#2193)
  chore(py): Bump `requests` lib version (#2194)
  doc: document pii fields (#1972)
  feat: Added cron specific endpoint (#2153)
  feat(profiling): Extract app identifier from app context (#2172)
  feat(ci): Add CI-check for pop-deploys  (#2184)
  ...
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.

Enforce limit:0 quotas on profiles
2 participants