-
Notifications
You must be signed in to change notification settings - Fork 91
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(metrics): Forward received_at field to Kafka #3561
Conversation
@@ -725,6 +725,7 @@ def send_buckets(buckets): | |||
produced_buckets.sort(key=lambda b: (b["name"], b["value"])) | |||
for bucket in produced_buckets: | |||
del bucket["timestamp"] | |||
del bucket["received_at"] |
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 test was very tricky to update, we might want to follow up with a PR to fix timestamps across all of our tests.
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.
Can we not do this better now?
|
||
|
||
class _WithinBounds: | ||
|
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.
Surprised the formatter doesn't complain about this newline
return f"{self._lower_bound} <= x <= {self._upper_bound}" | ||
|
||
|
||
def time_after(lower_bound): |
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 doesn't assert after, but after until now. I think making the upper bound on time_within
optional and defaulting to now is fine.
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.
Yeah, I was indeed thinking about a better name.
@@ -1873,3 +1890,48 @@ def test_metrics_with_denied_names( | |||
assert volume_metric["tags"]["outcome.reason"] == "denied-name" | |||
|
|||
assert "d:custom/memory_usage@byte" in metrics | |||
|
|||
|
|||
@pytest.mark.parametrize("mode", ["default", "chain"]) |
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.
Chain only is good enough imo
@@ -725,6 +725,7 @@ def send_buckets(buckets): | |||
produced_buckets.sort(key=lambda b: (b["name"], b["value"])) | |||
for bucket in produced_buckets: | |||
del bucket["timestamp"] | |||
del bucket["received_at"] |
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.
Can we not do this better now?
Co-authored-by: David Herberth <[email protected]>
This PR forwards the
received_at
field ofBucketMetadata
to Kafka. An important thing to notice is that the current Kafka schema allows additional fields: https://github.com/getsentry/sentry-kafka-schemas/blob/main/schemas/ingest-metrics.v1.schema.json meaning that this PR could be deployed without any side effects on the schemas, however, it would be best to mark the field as optional in the Kafka schema itself.The PR also introduces new facilities to relax timestamp assertions which are generally flaky.
Closes: #3515