ref(cbrs): integrate resource identifier and policy type into AllocationPolicy #7375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xurui-c
force-pushed
the
rachel/allocationpolicyref
branch
2 times, most recently
from
cef6448 to
920e872
Compare
xurui-c
force-pushed
the
rachel/allocationpolicyref
branch
from
920e872 to
36509d4
Compare
xurui-c
commented
Aug 31, 2025
|
|
||
| def to_dict(self) -> PolicyData: | ||
| base_data = super().to_dict() | ||
| return PolicyData(**base_data, query_type=self.query_type.value) # type: ignore |
Member
Author
There was a problem hiding this comment.
Without # type: ignore, it says Returning Any from function declared to return "PolicyData". This is because mypy can't guarantee that **base_data will satisfy all PolicyData requirements
Alternatively, I can just do
base_data = super().to_dict()
return PolicyData(
configurable_component_namespace=base_data["configurable_component_namespace"],
configurable_component_config_key=base_data["configurable_component_config_key"],
resource_identifier=base_data["resource_identifier"],
configurations=base_data["configurations"],
optional_config_definitions=base_data["optional_config_definitions"],
query_type=self.query_type.value,
)
3ee6f42 to
fdfde20
Compare
❌ 1 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
xurui-c
force-pushed
the
rachel/allocationpolicyref
branch
from
fdfde20 to
d1f1ec2
Compare
xurui-c
commented
Aug 31, 2025
|
|
||
| class ConfigurableComponentData(TypedDict): | ||
| configurable_component_namespace: str | ||
| configurable_component_config_key: str |
Member
Author
There was a problem hiding this comment.
I know at one point we thought that we should set component_name to the class name ie "BytesScannedWindowAllocationPolicy", so we wanted to "flip" component_name and config_key, but RegisteredClass has config_key representing the name of the class so I'd rather be consistent
xurui-c
force-pushed
the
rachel/allocationpolicyref
branch
from
d1f1ec2 to
bc89713
Compare
xurui-c
commented
Sep 1, 2025
| quota_info["quota_used"] = quota_allowance.quota_used | ||
| quota_info["quota_unit"] = quota_allowance.quota_unit | ||
| quota_info["suggestion"] = quota_allowance.suggestion | ||
| quota_info["storage_key"] = str(quota_and_policy.policy.storage_key) |
Member
Author
There was a problem hiding this comment.
As discussed in person, there was no vision between when using str(storage_key) (ie StorageKey.ERRORS) vs storage_key.value (ie errors), so I made everything use storage_key.value
If I don't do this, then ResourceIdentifier would have to keep track of the type of the underlying resource, because if the resource is StorageKey, then we'll want to use StoragaeKey's __repr__ method.
ResourceIdentifier should just take a string and when you pass in the storage key, convert to a string
xurui-c
force-pushed
the
rachel/allocationpolicyref
branch
from
f59dd2a to
343cd6d
Compare
xurui-c
commented
Sep 1, 2025
| ) | ||
| # make sure we always know which storage key we rejected a query from | ||
| allowance.explanation["storage_key"] = str(self._storage_key) | ||
| allowance.explanation["storage_key"] = self._resource_identifier.value |
Member
Author
There was a problem hiding this comment.
I could change the key to be "resource_identifier" instead of "storage_key" but I find it'd be more confusing to have "resource_identifier" in querylog/payloads but then "storage_key" in DD tags (changing the latter, I assume, would break a bunch of DD metrics and I'd rather move the project along...)
xurui-c
commented
Sep 2, 2025
| @property | ||
| def resource_identifier(self) -> ResourceIdentifier: | ||
| return ResourceIdentifier(self._storage_key) | ||
| def query_type(self) -> QueryType: |
Member
Author
There was a problem hiding this comment.
I debated having query_type be an argument to pass into the constructor, but AFAIK the only delete policy that exists is DeleteConcurrentRateLimitAllocationPolicy so this is the simplest way to change this
volokluev
approved these changes
Sep 2, 2025
Comment on lines
+432
to
+434
| @classmethod | ||
| def config_key(cls) -> str: | ||
| return cls.__name__ |
Member
There was a problem hiding this comment.
you said that RegisteredClass also has a config_key method but it represents a different thing. Maybe this function should be called something else that's more decriptive?
xurui-c
added a commit
that referenced
this pull request
Sep 8, 2025
https://linear.app/getsentry/issue/EAP-77/build-configuration-apis-for-routing-strategy This PR builds the necessary APIs and endpoints in Snuba Admin to retrieve configure routing strategies. This won't introduce any visual changes yet since it just establishes the endpoints DO NOT MERGE UNTIL: #7379, #7375 --------- Co-authored-by: Rachel Chen <rachelchen@PL6VFX9HP4.local> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allocation policies should be aware of whether they are a "select" policy or a "delete" policy, so that allocation policy payloads can be constructed from the object itself.
This PR also integrates resource identifiers into allocation policies and because allocation policies are no longer goverened by only storage keys, we get rid of the
storage_keyproperty