-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): accept str instead of int for extrapolation mode in validator #104015
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
base: master
Are you sure you want to change the base?
fix(aci): accept str instead of int for extrapolation mode in validator #104015
Conversation
| if name == extrapolation_mode: | ||
| extrapolation_mode = ExtrapolationMode(value) | ||
| break | ||
| if type(extrapolation_mode) is not ExtrapolationMode: |
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 you add a validate_extrapolation_mode() function to this validator that will transform the value into the enum class automatically? That way you won't need to do it here
| if extrapolation_mode is not None: | ||
| extrapolation_mode = ExtrapolationMode(extrapolation_mode) | ||
| extrapolation_mode_options = ExtrapolationMode.as_text_choices() | ||
| for name, value in extrapolation_mode_options: |
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.
Instead of this, you can add a classmethod from_str(self, name: str) function to ExtrapolationMode. We do that for some other enums. Then you can just do ExtrapolationMode.from_str(extrapolation_mode)
|
|
||
| def _validate_extrapolation_mode(self, extrapolation_mode: str) -> None: | ||
| if extrapolation_mode == ExtrapolationMode.SERVER_WEIGHTED.value: | ||
| if extrapolation_mode is not None and extrapolation_mode not in [ |
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 will also be simpler if you return the enum from the validator
| "Failed to send data to Seer, cannot update detector" | ||
| ) | ||
|
|
||
| numerical_extrapolation_mode = format_extrapolation_mode( |
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.
If you return an enum, you can instead do:
numerical_extrapolation_mode = data_source.get("extrapolation_mode", snuba_query.extrapolation_mode.value)
Slack thread for context: https://sentry.slack.com/archives/C07H2DHMSS0/p1764091469147239
Extrapolation mode would not be accepted as a string in the validator (which is what it gets passed to the backend as) so it was throwing an error on detector updates. Along with this I had to fix some tests and other logic that went into validating the extrapolation mode so we ensure we're comparing correct types.