-
-
Notifications
You must be signed in to change notification settings - Fork 63
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(sampling-in-storage): preflight mode #6979
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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 know this is a draft PR, but left a few comments.
5de25b9
to
17595fa
Compare
tests/web/rpc/v1/test_endpoint_trace_item_table/test_endpoint_trace_item_table.py
Outdated
Show resolved
Hide resolved
a705d07
to
3c08f19
Compare
131947e
to
9ede697
Compare
17b9316
to
40a75e0
Compare
ee8a60b
to
f4687c5
Compare
a73977b
to
074e4cd
Compare
074e4cd
to
930f8ad
Compare
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.
Looks good overall, let's just remove the seeded RNGs and just test that:
- the preflight mode returns fewer queries
- the response metadata is updated correctly
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.
LGTM mostly, added a few nits. Didn't look into the test failures, but are those happening because of the use of random
? Maybe better to simplify to not use random
if that is the cause of the failure.
@@ -104,6 +109,17 @@ | |||
} | |||
|
|||
|
|||
def add_tier_to_query_settings( |
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.
nit: naming could be changed to something more specific like set_sampling_tier_512
to be explicit. Also, I see that this function is called conditionally, so that condition can be handled inside this function instead of outside:
def set_sampling_tier_512(
request: TimeSeriesRequest | TraceItemTableRequest,
query_settings: HTTPQuerySettings,
) -> None:
if not request.meta.HasField("downsampled_storage_config"):
return # No downsampled storage config, so do nothing
if (
request.meta.downsampled_storage_config.mode
== DownsampledStorageConfig.MODE_PREFLIGHT
):
query_settings.set_sampling_tier(Tier.TIER_512)
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 the mode isn't preflight we shouldn't add tier 512 to query settings. the behavior of the function depends on the mode so I didn't want to specify which tier we're setting in the name
if request.meta.HasField("downsampled_storage_config"): | ||
add_tier_to_query_settings(request, query_settings) | ||
|
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 and other occurrences become:
set_sampling_tier_512(request, query_settings)
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.
mostly good., just a few comments
snuba/web/rpc/v1/resolvers/R_eap_spans/resolver_trace_item_table.py
Outdated
Show resolved
Hide resolved
5ca02ec
to
f08c3ae
Compare
f08c3ae
to
891a3fe
Compare
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.
lgtm
Implements preflight mode according to https://github.com/getsentry/eap-planning/issues/212
https://github.com/getsentry/eap-planning/issues/217
NOTE: DEPENDS ON #6981