Skip to content

Don't use post_init signal for initialize tracker#556

Merged
foarsitter merged 2 commits intojazzband:masterfrom
meanmail:master
Mar 21, 2024
Merged

Don't use post_init signal for initialize tracker#556
foarsitter merged 2 commits intojazzband:masterfrom
meanmail:master

Conversation

@meanmail
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

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
Copy Markdown

@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
Copy Markdown

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
Copy Markdown

@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
Copy Markdown
Contributor

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

@mlazowik
Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

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

@codecov
Copy link
Copy Markdown

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
Copy Markdown
Contributor

@meanmail then it is time to merge it!

@mga226
Copy link
Copy Markdown

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
@foarsitter
Copy link
Copy Markdown
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.

6 participants