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): Rate limit profiles when transaction was sampled #4195

Merged
merged 7 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
**Bug Fixes**:

- Accept incoming requests even if there was an error fetching their project config. ([#4140](https://github.com/getsentry/relay/pull/4140))
- Rate limit profiles when transaction was sampled. ([#4195](https://github.com/getsentry/relay/pull/4195))

**Features**:

Expand Down
30 changes: 30 additions & 0 deletions relay-server/src/utils/rate_limits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,15 @@ where
scoping.item(DataCategory::Profile),
summary.profile_quantity,
)?;

// Profiles can persist in envelopes without transaction if the transaction item
// was dropped by dynamic sampling.
if profile_limits.is_empty() && summary.event_category.is_none() {
profile_limits = self
.check
.apply(scoping.item(DataCategory::Transaction), 0)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to check for TransactionIndexed here, because indexed transactions will trigger the if let Some(category) = summary.event_category { branch above.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct that we don't have to check indexed here, but for the wrong reason.

It won't trigger the branch, because here we never have an event_category. But we don't have to check the indexed payload because I assume we don't want to enforce the indexed profile quota on profiles.

But this really is awkward and tricky, because ideally we should persist the information that we dropped indexed transactions on the profile (so it is treated like dynamic sampling), because then the profiling consumers later know that there won't be any transactions for the profiles (like it would be the case if the transaction is dynamically sampled).

As a side note, god damn I hate this function.

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't trigger the branch, because here we never have an event_category.

Let's break it down:

  1. If the dynamic sampling decision was "keep" and there is a limit on either transaction or transaction_indexed, the profile is already dropped here.
  2. If the dynamic sampling decision was "drop" and there is a limit on transaction, the new code drops the profile.
  3. If the dynamic sampling decision was "drop" and there is a limit on transaction_indexed, we are in undefined territory. I want to err on the side of not enforcing transaction_indexed on profiles, but it is awkward like you said, because in the "keep" case we would drop the profile.

As a side note, god damn I hate this function.

Yes.

}

enforcement.profiles = CategoryLimit::new(
DataCategory::Profile,
summary.profile_quantity,
Expand Down Expand Up @@ -1427,6 +1436,27 @@ mod tests {
);
}

#[test]
fn test_enforce_transaction_standalone_profile_enforced() {
// When the transaction is sampled, the profile survives as standalone.
let mut envelope = envelope![Profile];

let mut mock = MockLimiter::default().deny(DataCategory::Transaction);
let (enforcement, _) = enforce_and_apply(&mut mock, &mut envelope, None);

assert!(enforcement.profiles.is_active());
mock.assert_call(DataCategory::Profile, 1);
mock.assert_call(DataCategory::Transaction, 0);

assert_eq!(
get_outcomes(enforcement),
vec![
(DataCategory::Profile, 1),
(DataCategory::ProfileIndexed, 1),
]
);
}

#[test]
fn test_enforce_transaction_attachment_enforced_indexing_quota() {
let mut envelope = envelope![Transaction, Attachment];
Expand Down
70 changes: 70 additions & 0 deletions tests/integration/test_outcome.py
Original file line number Diff line number Diff line change
Expand Up @@ -1666,6 +1666,76 @@ def test_profile_outcomes_rate_limited(
assert outcomes == expected_outcomes, outcomes


@pytest.mark.parametrize("quota_category", ["transaction", "transaction_indexed"])
def test_profile_outcomes_rate_limited_when_dynamic_sampling_drops(
mini_sentry,
relay,
quota_category,
):
project_id = 42
project_config = mini_sentry.add_full_project_config(project_id)["config"]

project_config.setdefault("features", []).append("organizations:profiling")
project_config["quotas"] = [
{
"id": f"test_rate_limiting_{uuid.uuid4().hex}",
"categories": [quota_category],
"limit": 0,
"reasonCode": "profiles_exceeded",
}
]

config = {
"outcomes": {
"emit_outcomes": True,
"batch_size": 1,
"batch_interval": 1,
"aggregator": {
"bucket_interval": 1,
"flush_interval": 0,
},
},
"aggregator": {
"bucket_interval": 1,
"initial_delay": 0,
},
}

relay = relay(mini_sentry, options=config)

with open(
RELAY_ROOT / "relay-profiling/tests/fixtures/sample/v1/valid.json",
"rb",
) as f:
profile = f.read()

# Create an envelope with an invalid profile:
envelope = Envelope()
profile_item = Item(payload=PayloadRef(bytes=profile), type="profile")
profile_item.headers["sampled"] = False
envelope.add_item(profile_item)
relay.send_envelope(project_id, envelope)

if quota_category == "transaction":
(outcome1,) = mini_sentry.captured_outcomes.get(timeout=10)["outcomes"]
(outcome2,) = mini_sentry.captured_outcomes.get(timeout=1)["outcomes"]
outcome1, outcome2 = sorted([outcome1, outcome2], key=lambda o: o["category"])
assert outcome1["outcome"] == 2 # rate limited
assert outcome1["category"] == DataCategory.PROFILE
assert outcome1["quantity"] == 1
assert outcome2["outcome"] == 2 # rate limited
assert outcome2["category"] == DataCategory.PROFILE_INDEXED
assert outcome2["quantity"] == 1

assert mini_sentry.captured_events.empty()
else:
# Do not rate limit if there is only a transaction_indexed quota.
envelope = mini_sentry.captured_events.get(timeout=10)
assert envelope.items[0].headers["type"] == "profile"

assert mini_sentry.captured_outcomes.empty()


def test_global_rate_limit(
mini_sentry, relay_with_processing, metrics_consumer, outcomes_consumer
):
Expand Down
Loading