[CapMan visibility] surface CapMan summary in case of AllocationPolicyViolations #6052
Conversation
This file contains 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
evanh
reviewed
Jun 24, 2024
|
I originally wanted to abstract away the fact that summary is an addition to quota allowances, since the intention is that any time quota allowances is retrieved, a summary should come with it. I updated pr to be your approach since I believe it's cleaner and easier to understand |
evanh
reviewed
Jun 24, 2024
| @@ -170,16 +170,25 @@ def quota_allowance(self) -> dict[str, dict[str, Any]]: | |||
| dict[str, dict[str, Any]], self.extra_data.get("quota_allowances", {}) | |||
| ) | |||
|
|
|||
| @property | |||
| def summary(self) -> dict[str, Any]: | |||
| return cast(dict[str, Any], self.extra_data.get("summary")) | |||
There was a problem hiding this comment.
Is there a case where the summary key won't be in the extra_data? If so, I don't think this typing holds true. In that case the function will return None.
There was a problem hiding this comment.
No, there will always be a summary
xurui-c
force-pushed
the
rachel/addSummaryToSentryIssues
branch
from
2e0f2cf to
c7dbfd5
Compare
xurui-c
force-pushed
the
rachel/addSummaryToSentryIssues
branch
from
c7dbfd5 to
574a23a
Compare
xurui-c
commented
Jun 25, 2024
snuba/web/views.py
Outdated
| @@ -418,7 +418,8 @@ def dataset_query( | |||
| { | |||
| "error": details, | |||
| "timing": timer.for_json(), | |||
| "quota_allowance": getattr(cause, "quota_allowance", {}), | |||
| "quota_allowance": getattr(cause, "quota_allowance", {}) | |||
| | getattr(cause, "summary", {}), | |||
There was a problem hiding this comment.
@evanh I initially had the two concepts in one dictionary because I was trying to avoid this. By previous design the summary field is a part of the quota_allowance dictionary, so if we kept them as separate attributes inside AllocationPolicyViolations, we have to re-combine them outside like this. I feel like this then becomes inconsistent with cases where the query is successful, and the quota_allowance dictionary contains the summary field
xurui-c
force-pushed
the
rachel/addSummaryToSentryIssues
branch
from
6e59e53 to
f491fa6
Compare
|
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2467 tests with View the full list of failed testspytest
|
volokluev
approved these changes
Jun 25, 2024
| @@ -158,28 +158,33 @@ class AllocationPolicyViolations(SerializableException): | |||
| """ | |||
|
|
|||
| def __str__(self) -> str: | |||
| return f"{self.message}, details: {self.violations}" | |||
| return f"{self.message}, info: {self.violations | self.summary}" | |||
There was a problem hiding this comment.
Suggested change
| return f"{self.message}, info: {self.violations | self.summary}" | |
| return f"{self.message}, info: {"violations" self.violations, "summary": self.summary}" |
Comment on lines
176
to
178
| summary = self.quota_allowance.get("summary") | ||
| assert summary is not None | ||
| return {"summary": summary} |
There was a problem hiding this comment.
Suggested change
| summary = self.quota_allowance.get("summary") | |
| assert summary is not None | |
| return {"summary": summary} | |
| return self.quota_allowance.get("summary", {}) |
xurui-c
force-pushed
the
rachel/addSummaryToSentryIssues
branch
from
f491fa6 to
72675b4
Compare
xurui-c
changed the title
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.
https://getsentry.atlassian.net/browse/SNS-2798
Allocation policies are our mechanism for doing traffic management for Snuba queries. On Sentry issues webpage, the metadata regarding allocation policies are not surfaced.
In this PR, we relay the metadata to Sentry issues webpage so that they are displayed in that same section.