-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(derived_code_mappings): Rename to Automatic Source Code Configuration #83313
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #83313 +/- ##
==========================================
+ Coverage 78.15% 87.54% +9.38%
==========================================
Files 9403 9405 +2
Lines 537402 537615 +213
Branches 21175 21175
==========================================
+ Hits 420019 470660 +50641
+ Misses 117035 66607 -50428
Partials 348 348 |
@@ -13,6 +13,8 @@ | |||
logger = logging.getLogger(__name__) | |||
logger.setLevel(logging.INFO) | |||
|
|||
SUPPORTED_LANGUAGES = ["javascript", "python", "node", "ruby", "php", "go", "csharp"] |
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 constant is related to code mappings, thus, placing it here.
@@ -109,7 +111,9 @@ def __init__(self, frame_file_path: str) -> None: | |||
def __repr__(self) -> str: | |||
return f"FrameFilename: {self.full_path}" | |||
|
|||
def __eq__(self, other) -> bool: | |||
def __eq__(self, other: object) -> bool: |
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.
Improving the typing.
.github/CODEOWNERS
Outdated
@@ -530,6 +530,7 @@ tests/sentry/api/endpoints/test_organization_dashboard_widget_details.py @ge | |||
/src/sentry/event_manager.py @getsentry/issues | |||
/src/sentry/eventstore/models.py @getsentry/issues | |||
/src/sentry/grouping/ @getsentry/issues | |||
/src/sentry/issues/ @getsentry/issues |
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 had never added this. Tagging along in this PR.
Queue("derive_code_mappings", routing_key="derive_code_mappings"), | ||
Queue("auto_source_code_config", routing_key="auto_source_code_config"), |
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.
Once it gets created, we can switch over to it in a follow-up PR.
name="sentry.tasks.derive_code_mappings.derive_code_mappings", | ||
queue="derive_code_mappings", | ||
name="sentry.tasks.auto_source_code_config.derive_code_mappings", | ||
queue="derive_code_mappings", # XXX: To be renamed to auto_source_code_config |
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 will switch over to the new queue on a follow-up PR.
@@ -71,8 +72,6 @@ | |||
"sentry.tasks.summaries.weekly_reports.prepare_organization_report": 0.1 | |||
* settings.SENTRY_BACKEND_APM_SAMPLING, | |||
"sentry.profiles.task.process_profile": 0.1 * settings.SENTRY_BACKEND_APM_SAMPLING, | |||
"sentry.tasks.derive_code_mappings.process_organizations": settings.SAMPLED_DEFAULT_RATE, |
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.
process_organizations
was removed a long time ago.
@@ -400,7 +400,13 @@ def test_format_comment(self): | |||
] | |||
|
|||
formatted_comment = format_comment(issues) | |||
expected_comment = "## Suspect Issues\nThis pull request was deployed and Sentry observed the following issues:\n\n- ‼️ **TypeError** `sentry.tasks.derive_code_mappings.derive_code_m...` [View Issue](https://sentry.sentry.io/issues/?referrer=github-pr-bot)\n- ‼️ **KafkaException** `query_subscription_consumer_process_message` [View Issue](https://sentry.sentry.io/stats/?referrer=github-pr-bot)\n\n<sub>Did you find this useful? React with a 👍 or 👎</sub>" | |||
expected_comment = """## Suspect Issues |
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.
Splitting it over since it is much easier to read.
name="sentry.tasks.derive_code_mappings.derive_code_mappings", | ||
queue="derive_code_mappings", | ||
name="sentry.tasks.auto_source_code_config.derive_code_mappings", |
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 believe this name needs to remain constant as well
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.
You can't remove/change the name
of a task if there are any spawn calls left in the code that is currently in production, as old web/workers will be spawning the old task, and that new task cannot be handled by new pods.
Instead, you should leave add the new task definition. You'll also need to retain the existing instrumented_task
decorator and function signature. The 'old' function can call the new function to not duplicate logic.
If you want to avoid any task drops, you'd also need to not call the new task in these changes, as a new worker could spawn tasks that cannot be handled by an 'old' worker. Instead you could deploy the new task, and then separately deploy changes that start using the new task name.
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.
Thank you both! I will look into it.
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.
the rename lgtm other than the task already mentioned
@@ -89,6 +92,20 @@ def process_error(error: ApiError, extra: dict[str, str]) -> None: | |||
def derive_code_mappings( | |||
project_id: int, | |||
data: NodeData, | |||
) -> None: | |||
derive_code_mappings_new(project_id, data) |
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.
@markstory @asottile-sentry
Let me know if I understood your comments correctly.
default_retry_delay=60 * 10, | ||
max_retries=3, | ||
) | ||
def derive_code_mappings_new( |
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.
In a follow-up PR I will update the queue and name for this function.
src/sentry/tasks/post_process.py
Outdated
@@ -1016,7 +1017,7 @@ def process_code_mappings(job: PostProcessJob) -> None: | |||
else: | |||
return | |||
|
|||
derive_code_mappings.delay(project.id, event.data) | |||
auto_source_code_config.delay(project.id, event.data) |
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.
You could drop task activations here. During a deploy workers are replaced incrementally, and a 'new' worker could spawn this task, which is processed by an 'old' worker. The old worker won't have the new task definition and the activation will be lost.
What I've done in the past for this kind of situation is use an option to choose which task is scheduled. Once all workers are restart, the option can be flipped and the new task will be scheduled. This helps clean up to be done in one pull request and deploy instead of needing to use two or three.
@@ -34,6 +34,8 @@ def register_temporary_features(manager: FeatureManager): | |||
|
|||
# Enables user registration. | |||
manager.add("auth:register", SystemFeature, FeatureHandlerStrategy.INTERNAL, default=True) | |||
# Switch to new queue for auto source code config | |||
manager.add("new-auto-source-code-config-queue", SystemFeature, FeatureHandlerStrategy.FLAGPOLE) |
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 is the option to control the queue.
if features.has("new-auto-source-code-config-queue"): | ||
auto_source_code_config.delay(project.id, data=event.data, event_id=event.event_id) | ||
else: | ||
derive_code_mappings.delay(project.id, data=event.data, event_id=event.event_id) |
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.
@markstory please let me know if I got this right.
@@ -375,6 +375,28 @@ def test_skipping_an_issue_doesnt_mark_it_processed(self, mock_derive_code_mappi | |||
self._call_post_process_group(charlie_event2) | |||
assert mock_derive_code_mappings.delay.call_count == 2 | |||
|
|||
# XXX: Delete this test once we've migrated |
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.
@markstory these two tests will be deleted in the future but it tests that the right queue is used.
The original name of the feature was Derived Code Mappings, however, we have plans to expand it to also automatically do the following configurations beyond code mappings:
There is also an upcoming effort from the integrations to bring feature parity across integrations, thus, this is a good time to decouple the code.
This first change focuses on renaming and moving files around while the following pull request will focus on refactoring and decoupling the code.