Skip to content

Conversation

iambriccardo
Copy link
Contributor

@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
produced_buckets.sort(key=lambda b: (b["name"], b["value"]))
for bucket in produced_buckets:
del bucket["timestamp"]
del bucket["received_at"]
Copy link
Contributor 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?



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
Contributor 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.

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

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
@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