-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(on-demand): Add feature flag check before cardinality check #98726
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(on-demand): Add feature flag check before cardinality check #98726
Conversation
is_task: bool = False, | ||
widget_query: DashboardWidgetQuery | None = None, | ||
) -> dict[str, str]: | ||
if not features.has("organizations:on-demand-metrics-extraction-widgets", organization): |
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.
@k-fish would you be able to advise if this is okay for us to do? I suppose when we migrate fully to EAP then we can get rid of this, but in an effort to make some easy progress for other users, I figured we could just feature flag this check
It seems like we wanted to evaluate it even without the feature flag so when we rolled out, people's queries were good to go
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.
We can get rid of the flag if we confirm if any of the orgs are not still on am1 / their widgets work on EAP
In theory yes, we were supposed to check so that we could roll it out for every org, but am3 came along. We're okay skipping it for extraction widgets.
Can you copy the test with the flag to assert the old behaviour? otherwise it's fine I think
is_task: bool = False, | ||
widget_query: DashboardWidgetQuery | None = None, | ||
) -> dict[str, str]: | ||
if not features.has("organizations:on-demand-metrics-extraction-widgets", organization): |
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.
We can get rid of the flag if we confirm if any of the orgs are not still on am1 / their widgets work on EAP
In theory yes, we were supposed to check so that we could roll it out for every org, but am3 came along. We're okay skipping it for extraction widgets.
Can you copy the test with the flag to assert the old behaviour? otherwise it's fine I think
Some users are experiencing high latency due to this cardinality check. Since metrics extraction for widgets has paused and we're in the process of migrating users to spans/EAP, I figured we could avoid running this check for users that do not have the feature flag if we're not planning to continue rolling it out.
e.g. this trace shows 15s waiting for the cardinality check for an org that doesn't have access