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

logging timestamps #1121

Merged
merged 1 commit into from
Feb 13, 2025
Merged

logging timestamps #1121

merged 1 commit into from
Feb 13, 2025

Conversation

jjanowsk
Copy link
Collaborator

@jjanowsk jjanowsk commented Feb 12, 2025

Add granular timestamps to logs

There is a suspicion that our logs can lock from time to time.
Currently the timestamp in our logs comes from the very end of log
processing right before sending it to the writer thread which executes
application callback. To check if we do not have any locking inside
logging machinery on call site let's add two timestamps to our logs. One
directly from the call site and second in formatter. Third is already
there and comes from writter as explained above.

This is a temporary commit meant to be reverted once we have results.

☑️ Definition of Done checklist

  • Commit history is clean (requirements)
  • README.md is updated
  • Functionality is covered by unit or integration tests

@jjanowsk jjanowsk force-pushed the jj_logging_timestamps branch from d110943 to c9e921c Compare February 12, 2025 13:55
@jjanowsk jjanowsk force-pushed the jj_logging_timestamps branch from c9e921c to 4692ea6 Compare February 12, 2025 20:32
@jjanowsk jjanowsk force-pushed the jj_logging_timestamps branch from 4692ea6 to 00e102f Compare February 13, 2025 07:16
@jjanowsk jjanowsk force-pushed the jj_logging_timestamps branch from 00e102f to 71a6670 Compare February 13, 2025 10:52
@jjanowsk jjanowsk marked this pull request as ready for review February 13, 2025 10:54
@jjanowsk jjanowsk requested a review from a team as a code owner February 13, 2025 10:54
@Hasan6979
Copy link
Contributor

+2
It would be worth to add dns or something with branch name, so that when we revert the commits -> easier to find. I feel that we might have to make more temp branches than we already have.

There is a suspicion that our logs can lock from time to time.
Currently the timestamp in our logs comes from the very end of log
processing right before sending it to the writer thread which executes
application callback. To check if we do not have any locking inside
logging machinery on call site let's add two timestamps to our logs. One
directly from the call site and second in formatter. Third is already
there and comes from writter as explained above.

This is a temporary commit meant to be reverted once we have results.
Copy link
Contributor

@LukasPukenis LukasPukenis left a comment

Choose a reason for hiding this comment

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

An idea: we can create a ticket about reverting this commit and link in TODO item in the code. This way we can leave it even for longer if needed with no worries about forgetting it(highly unlikeable though).

+1

@jjanowsk jjanowsk enabled auto-merge February 13, 2025 12:02
@jjanowsk jjanowsk added this pull request to the merge queue Feb 13, 2025
@tomaszklak
Copy link
Contributor

@jjanowsk
Copy link
Collaborator Author

Maybe this mutex is causing problem: https://github.com/search?q=repo%3ANordSecurity%2Flibtelio%20LAST_LOG_STATUS&type=code ?

It might. It's the only lock I saw on the call site. But I would like to first confirm that what we saw was actually a lock in logging and not somewhere else. BTW since I think you're the implementor of the logging thread I have a question: Is there a reason why this filtering and censoring is done on the call site instead of inside the logging thread?

@tomaszklak
Copy link
Contributor

Maybe this mutex is causing problem: https://github.com/search?q=repo%3ANordSecurity%2Flibtelio%20LAST_LOG_STATUS&type=code ?

It might. It's the only lock I saw on the call site. But I would like to first confirm that what we saw was actually a lock in logging and not somewhere else. BTW since I think you're the implementor of the logging thread I have a question: Is there a reason why this filtering and censoring is done on the call site instead of inside the logging thread?

No real reason. The offloading to separate thread was done due to slow I/O (via python) - I wasn't expecting (and didn't see any symptoms) the processing (which was already present for quite some time) to be an issue. So I think it perfectly valid offload it to logger thread.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 13, 2025
@jjanowsk jjanowsk added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit 4370150 Feb 13, 2025
55 checks passed
@jjanowsk jjanowsk deleted the jj_logging_timestamps branch February 13, 2025 15:23
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.

4 participants