-
Notifications
You must be signed in to change notification settings - Fork 94
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(profiling): add namespace for profile metrics #3229
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be worth adding an integration test, like this one:
relay/tests/integration/test_metrics.py
Lines 169 to 204 in 3dff617
def test_metrics(mini_sentry, relay): | |
relay = relay(mini_sentry, options=TEST_CONFIG) | |
project_id = 42 | |
mini_sentry.add_basic_project_config(project_id) | |
timestamp = int(datetime.now(tz=timezone.utc).timestamp()) | |
metrics_payload = ( | |
f"transactions/foo:42|c|T{timestamp}\ntransactions/bar:17|c|T{timestamp}" | |
) | |
relay.send_metrics(project_id, metrics_payload) | |
envelope = mini_sentry.captured_events.get(timeout=3) | |
assert len(envelope.items) == 1 | |
metrics_item = envelope.items[0] | |
assert metrics_item.type == "metric_buckets" | |
received_metrics = json.loads(metrics_item.get_bytes().decode()) | |
received_metrics = sorted(received_metrics, key=lambda x: x["name"]) | |
assert received_metrics == [ | |
{ | |
"timestamp": timestamp, | |
"width": 1, | |
"name": "c:transactions/bar@none", | |
"value": 17.0, | |
"type": "c", | |
}, | |
{ | |
"timestamp": timestamp, | |
"width": 1, | |
"name": "c:transactions/foo@none", | |
"value": 42.0, | |
"type": "c", | |
}, | |
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make sure every other piece of the pipeline already supports the namespace!
Are Sentry and the consumers already able to process the profile metrics without crashing?
@viglia I am just a bit paranoid, when we add it to Relay this means anyone can ingest these messages on any of our instances. I don't know what needs to be added to Sentry, but it seems like there were no changes made to rate and cardinality limits, which eventually should be added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we link in the description to the PRs/code where the namespace has been added in sentry/snuba/etc? Thanks beforehand!
@Dav1dde keep in mind that setting those limits is optional.
So we have a default rate limiting value, both for global and for the per-org Should we need customized values, we'll only find out with time and can make those changes to enforce different limits then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dav1dde keep in mind that setting those limits is optional.
Maybe, but these settings exist to protect our infrastructure. And in the incident case the necessary code in Sentry is missing to even configure the limits.
These two configs you pointed out are not the correct ones.
I urge you to follow up on these things, this is incident territory.
I don't want to block you, the Relay changes are fine the, just please drop the error.
The Rest I don't want to worry about.
yes, that's why there are default cardinality & write limits that are shared among use cases that don't specify a custom one.
Before merging I'll follow up with |
Sentry PR that added the
profles
use case: https://github.com/getsentry/sentry/pull/65152/files