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

ref(metrics): Create extracted metrics summary for sets with min/max/sum of 0 #3853

Closed
wants to merge 1 commit into from

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Jul 23, 2024

It seems to me that snuba consumer expects those fields and fails when those fields are not present, and the metric summary is than never persisted in the database (metric summary table doesn't allow this fields to be NULL, so I think setting those fields to 0 is the best solution)

Created by Relay:
image

Sent from the SDK:
image

PR creates the set summary using the same 0 defaults.

#skip-changelog

@Dav1dde Dav1dde requested a review from a team as a code owner July 23, 2024 13:08
@Dav1dde Dav1dde force-pushed the dav1d/metric-summaries-sets branch from 41788ca to 294d48d Compare July 23, 2024 13:17
Comment on lines +103 to +105
min: Some(0.into()),
max: Some(0.into()),
sum: Some(0.into()),
Copy link
Member

Choose a reason for hiding this comment

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

Should we type these fields to be non-optional, if the consumer cannot handle them otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to the Kafka schema, any of these is required. So technically it should work if we just send min: Some(0) but keep the others None ... Unfortunately this is quite hard to express in the typesystem.

@phacops
Copy link
Contributor

phacops commented Jul 24, 2024

The Kafka schema was updated to make min, max, sum optional and make count required 0. Now updating the consumer 1 to support the optional values.

After this, I don't think this PR will be needed anymore.

@Dav1dde Dav1dde closed this Jul 24, 2024
@Dav1dde
Copy link
Member Author

Dav1dde commented Jul 24, 2024

Thanks Pierre!

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.

3 participants