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

Improve handling of newlines in log messages #596

Merged
merged 4 commits into from
Jan 13, 2024
Merged

Conversation

5tefan
Copy link
Contributor

@5tefan 5tefan commented Dec 19, 2023

(Apologies for not starting a discussion as requested in contributing guidelines... let's have the discussion here and reject this PR if it doesn't align with your vision.)

Summary

Improves the newline handling in log forwarding from workers so that it does not always introduce newlines.

Problem

Due to some unknown change in our setup, we started seeing log messages from workers split into multiple lines:

image

Perhaps we are flushing log message more frequently than expected by dramatiq... and the flushes are not aligned with newlines.

Solution

After some digging, we found that the newlines are inserted by dramatiq here:

dramatiq/dramatiq/cli.py

Lines 322 to 338 in a30b787

while event.poll():
# StreamHandler writes newlines into the pipe separately
# from the actual log entry; to avoid back-to-back newlines
# in the events pipe (causing multiple entries on a single
# line), discard newline-only data from the pipe
try:
data = event.recv_bytes()
except EOFError:
event.close()
raise
data = data.decode("utf-8", errors="replace").rstrip("\n")
if not data:
continue
log_file.write(data + "\n")
log_file.flush()

This code always adds a newline, even if the rstrip did not strip one out.

Given the comment above this code section explaining the goal:

discard newline-only data from the pipe

I believe this revision accomplishes this better by only using rstrip for the continue condition (to not pass blank lines),but otherwise passing the log without modification.

@Bogdanp
Copy link
Owner

Bogdanp commented Jan 13, 2024

Thanks! I'll try to review this PR this weeked or the next. The code in question does look like it assumes line buffering, so your setup probably changed to some other buffer mode for whatever reason.

@Bogdanp Bogdanp merged commit 2808d85 into Bogdanp:master Jan 13, 2024
11 checks passed
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.

2 participants