Skip to content
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

Don't use post_init signal for initialize tracker #556

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

meanmail
Copy link
Contributor

@meanmail meanmail commented Feb 5, 2023

Problem

In one of the latest versions, sentry-sdk added logging of the launch of django signal handlers. Due to the fact that the post_init signal handler is used to initialize the tracker, this leads to a significant decrease in performance, and also occupies the sentry logs with useless entries, which makes it impossible to analyze performance in Sentry

image

Solution

Don't use post_init signal for initialize tracker

Commandments

  • Write PEP8 compliant code.
  • Cover it with tests.
  • Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
  • Pay attention to backward compatibility, or if it breaks it, explain why.
  • Update documentation (if relevant).

@foarsitter
Copy link
Contributor

@yuekui can you review this PR? Looks like you and @meanmail are both fixen the same issue.

@@ -340,7 +340,7 @@ def finalize_class(self, sender, **kwargs):
wrapped_descriptor = wrapper_cls(field_name, descriptor, self.attname)
setattr(sender, field_name, wrapped_descriptor)
self.field_map = self.get_field_map(sender)
models.signals.post_init.connect(self.initialize_tracker)
self.patch_init(sender)
self.model_class = sender
Copy link

@yuekui yuekui Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could safely remove this property model_class since not using signal anymore, refer to 98f078d

@yuekui
Copy link

yuekui commented Jun 16, 2023

@yuekui can you review this PR? Looks like you and @meanmail are both fixen the same issue.

Sure, this patch also looks good to me, just a minor comment.

@Alireza2n
Copy link

@meanmail Thank you for the PR, I'm experiencing the same issue as yours.
Is there anything I can do to help get this PR merged?

@foarsitter
Copy link
Contributor

If you can test this change in production it would be helpful.

@mlazowik
Copy link

mlazowik commented Dec 7, 2023

For anyone else who looks into this – in the meantime you might be interested in getsentry/sentry-python#1929

@meanmail meanmail reopened this Mar 11, 2024
@meanmail
Copy link
Contributor Author

We've been using this change in production for over a year now. There were no problems

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (46d3392) to head (2bbbfcb).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
- Coverage   95.10%   95.01%   -0.09%     
==========================================
  Files           6        6              
  Lines         796      803       +7     
==========================================
+ Hits          757      763       +6     
- Misses         39       40       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@foarsitter
Copy link
Contributor

@meanmail then it is time to merge it!

@mga226
Copy link

mga226 commented Mar 20, 2024

Is there a timeline for this being ready to use?

@foarsitter foarsitter merged commit 473ee74 into jazzband:master Mar 21, 2024
13 of 14 checks passed
@foarsitter
Copy link
Contributor

@mga226 it is a jazzband project so feel free to contribute.

@foarsitter foarsitter added this to the 4.5.0 milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants