-
Notifications
You must be signed in to change notification settings - Fork 781
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_server): do not invalidate legacy report types #1532
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.
I found the new logic a lot more confusing than the original one.
I propose we restore the old logic. We can still process the report like before in terms of validation. But just drop them instead.
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.
I'll address the other comments once I get clarity on whether we need to drop the access key reports or not.
Actually, nevermind this - I didn't initially see your top-level comment re: dropping them. |
for (const userReport of testObject.userReports) { | ||
// Check that `bytesTransferred` is a number between min and max transfer limits | ||
// We require at least the userId or the country to be set. | ||
if (!userReport.userId && (userReport.countries?.length ?? 0) === 0) { |
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.
I think this check is unnecessary, since the body here already knows how to trigger on the different cases. It's no big issue, but it's kind of duplicated logic. And if we add new report types in the future, this will have to change.
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.
It knows how to handle those properties, but it doesn't verify that the report has at least 1 of them. I'll remove it and change the model to make it more forgiving, though I don't think we ever allowed neither to be set?
Servers released before #1242 or #1529 will continue to send reports with multiple
countries
or auserId
property respectively. This change reinstates backwards compatibility (broken in #1528) and makes support for these old types more explicit to help future devs.