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

feat(metrics): Forward received_at field to Kafka #3561

Merged
merged 24 commits into from
May 22, 2024

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented May 7, 2024

This PR forwards the received_at field of BucketMetadata 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

@iambriccardo iambriccardo requested a review from Dav1dde May 10, 2024 08:37
@iambriccardo iambriccardo marked this pull request as ready for review May 10, 2024 08:37
@iambriccardo iambriccardo requested a review from a team as a code owner May 10, 2024 08:37
@@ -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"]
Copy link
Member Author

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.

Copy link
Member

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?

relay-server/src/services/store.rs Outdated Show resolved Hide resolved


class _WithinBounds:

Copy link
Member

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):
Copy link
Member

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.

Copy link
Member Author

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"])
Copy link
Member

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"]
Copy link
Member

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?

@iambriccardo iambriccardo merged commit 5b8198b into master May 22, 2024
22 checks passed
@iambriccardo iambriccardo deleted the riccardo/feat/forward-received-at branch May 22, 2024 07:08
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.

Propagate received_at field of BucketMetadata to Kafka and consume it
3 participants