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

Issue/executor log cleanup #8000

Closed
wants to merge 3 commits into from
Closed

Conversation

wouterdb
Copy link
Contributor

Description

Final log cleanup, based on working with systemtenant

closes #7692

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

@wouterdb wouterdb requested a review from hlloreda August 23, 2024 13:09
@wouterdb wouterdb self-assigned this Aug 23, 2024
@@ -245,7 +245,7 @@ async def get(self, member_id: TPoolID) -> TPoolMember:
"""
Returns a valid pool member for the given id
"""
LOGGER.debug("%s: requesting %s", self.my_name(), self.render_id(member_id))
LOGGER.log(LOG_LEVEL_TRACE, "%s: requesting %s", self.my_name(), self.render_id(member_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between this log level trace and the debug level? I'm unsure to see when to use one over the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trace is one level lower than debug.

The reason I reduced this that for every call to an executor (and they happen A LOT),

  1. for a cache hit we get two log lines at debug level, extremely close together. (requesting and found)
  2. for a cache miss we also get 2 (requested and creating) but not one when it is done creating.

So this log line is at trace, in order to debug locking issues, which should be extremely rare.

@wouterdb wouterdb requested a review from hlloreda August 23, 2024 14:34
@wouterdb wouterdb added the merge-tool-ready This ticket is ready to be merged in label Aug 23, 2024
@inmantaci
Copy link
Contributor

Processing this pull request

@inmantaci
Copy link
Contributor

Merged into branches master in ce4de1a

inmantaci pushed a commit that referenced this pull request Aug 23, 2024
# Description

Final log cleanup, based on working with systemtenant

closes #7692

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [x] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)
@inmantaci inmantaci closed this Aug 23, 2024
@inmantaci inmantaci deleted the issue/executor_log_cleanup branch August 23, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-tool-ready This ticket is ready to be merged in
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executor testing
3 participants