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

Alternative Modified Backport of #4619 #4660

Merged
merged 3 commits into from
Feb 2, 2022

Conversation

nathaniel-may
Copy link
Contributor

@nathaniel-may nathaniel-may commented Feb 1, 2022

resolves #4569

###Description

This commit defers the expensive function dump_graph to only be called in the event the value is going to be logged. Without this change, users always generate this expensive graph regardless of the value of the flag --log-cache-events. This is an alternative solution to the one proposed in #4656.

This backport of #4619 is separate from the original commit so that we do not have to backport #4505 which introduced mashumaro as the internal serializer for the events module. In order to keep this backport simple, I implemented it by adding a new function fire_event_if that only constructs the event and logs it if the condition is met. This keeps logging concerns within the logging module while exposing conditional logging at call sites. Tests for the original commit are not directly applicable anymore because we are not using fire_event anymore.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@nathaniel-may nathaniel-may requested review from leahwicz and a team as code owners February 1, 2022 22:50
@nathaniel-may nathaniel-may requested a review from a team February 1, 2022 22:50
@nathaniel-may nathaniel-may requested a review from a team as a code owner February 1, 2022 22:50
@nathaniel-may nathaniel-may requested review from VersusFacit and removed request for a team February 1, 2022 22:50
@cla-bot cla-bot bot added the cla:yes label Feb 1, 2022
@nathaniel-may nathaniel-may changed the base branch from main to 1.0.latest February 1, 2022 22:50
@nathaniel-may nathaniel-may mentioned this pull request Feb 1, 2022
4 tasks
@nathaniel-may nathaniel-may requested review from emmyoop and removed request for VersusFacit and a team February 2, 2022 15:44
Copy link
Contributor

@leahwicz leahwicz left a comment

Choose a reason for hiding this comment

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

LGTM. My only thing would be could we add a test for this just to cover all the bases?

@nathaniel-may
Copy link
Contributor Author

@leahwicz I just added a simple unit test for fire_event_if which shows that if you use that function properly, it CAN have the behavior we want. I looked into writing an integration test to determine if it IS being used correctly, but I would need to add another event to log when dump_graph is being called since I don't see a way to access that state in a unit or integration test.

In the interest of not adding new behavior to a patch release, I think we shouldn't add the new log event for the test. Is the test I've added enough or should we look into other avenues?

@leahwicz
Copy link
Contributor

leahwicz commented Feb 2, 2022

@nathaniel-may I think this is good enough. Just verifies the new method which I'm happy with. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants