Skip to content

fix(sentry): factor out redis-related logging#563

Merged
mfocko merged 1 commit into
packit:mainfrom
mfocko:feat/sentry/factor-out-redis-logging
Jun 4, 2026
Merged

fix(sentry): factor out redis-related logging#563
mfocko merged 1 commit into
packit:mainfrom
mfocko:feat/sentry/factor-out-redis-logging

Conversation

@mfocko
Copy link
Copy Markdown
Member

@mfocko mfocko commented Jun 3, 2026

As I have enabled log-collection to Sentry, I have noticed that the logs are massively spammed by the queue-processing logging, e.g.,

• “Waiting for tasks from rebase_queue_c10s (timeout: 30s)...”
• “No tasks received, continuing to wait...”

Therefore I introduce a separate logger for these highly recurrent messages that can be explicitly ignored by Sentry.

TODO

  • I have also noticed that logging.getLogger(__name__) is not very helpful, since in all cases it gets expanded to just __main__, I think it would be for the better to adjust that

As I have enabled log-collection to Sentry, I have noticed that the logs
are massively spammed by the queue-processing logging, e.g.,

• “Waiting for tasks from rebase_queue_c10s (timeout: 30s)...”
• “No tasks received, continuing to wait...”

Therefore I introduce a separate logger for these highly recurrent
messages that can be explicitly ignored by Sentry.

Signed-off-by: Matej Focko <mfocko@packit.dev>
@mfocko mfocko self-assigned this Jun 3, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a dedicated redis_logger ('agent.redis') across multiple agents and configures Sentry to ignore it, aiming to reduce log noise from repetitive Redis polling. The reviewer recommends keeping the connection and task receipt log messages on the main logger rather than moving them to redis_logger. Since these events are not spammy and occur only at startup or when a task is actually dispatched, retaining them on the main logger preserves valuable Sentry breadcrumbs for debugging, while still successfully filtering out the idle polling logs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +1417 to +1429
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {backport_queue}"
)

while True:
logger.info(f"Waiting for tasks from {backport_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {backport_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([backport_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue

_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The Connected to Redis... and Received task from queue. log messages are not highly recurrent or spammy. The connection log occurs only once at startup, and the task receipt log occurs only when a task is actually dispatched. Keeping these on the main logger is highly beneficial for observability and debugging in Sentry (e.g., as breadcrumbs leading up to an error), whereas only the idle polling logs (Waiting for tasks... and No tasks received...) should be ignored.

Suggested change
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {backport_queue}"
)
while True:
logger.info(f"Waiting for tasks from {backport_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {backport_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([backport_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {backport_queue}"
)
while True:
redis_logger.info(f"Waiting for tasks from {backport_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([backport_queue], timeout=30))
if element is None:
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")

Comment on lines +530 to +542
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebase_queue}"
)

while True:
logger.info(f"Waiting for tasks from {rebase_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {rebase_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebase_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue

_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The Connected to Redis... and Received task from queue. log messages are not highly recurrent or spammy. The connection log occurs only once at startup, and the task receipt log occurs only when a task is actually dispatched. Keeping these on the main logger is highly beneficial for observability and debugging in Sentry (e.g., as breadcrumbs leading up to an error), whereas only the idle polling logs (Waiting for tasks... and No tasks received...) should be ignored.

Suggested change
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebase_queue}"
)
while True:
logger.info(f"Waiting for tasks from {rebase_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {rebase_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebase_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebase_queue}"
)
while True:
redis_logger.info(f"Waiting for tasks from {rebase_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebase_queue], timeout=30))
if element is None:
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")

Comment on lines +337 to +349
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebuild_queue}"
)

while True:
logger.info(f"Waiting for tasks from {rebuild_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {rebuild_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebuild_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue

_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The Connected to Redis... and Received task from queue. log messages are not highly recurrent or spammy. The connection log occurs only once at startup, and the task receipt log occurs only when a task is actually dispatched. Keeping these on the main logger is highly beneficial for observability and debugging in Sentry (e.g., as breadcrumbs leading up to an error), whereas only the idle polling logs (Waiting for tasks... and No tasks received...) should be ignored.

Suggested change
redis_logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebuild_queue}"
)
while True:
logger.info(f"Waiting for tasks from {rebuild_queue} (timeout: 30s)...")
redis_logger.info(f"Waiting for tasks from {rebuild_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebuild_queue], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")
redis_logger.info("Received task from queue.")
logger.info(
f"Connected to Redis, max retries set to {max_retries}, listening to queue: {rebuild_queue}"
)
while True:
redis_logger.info(f"Waiting for tasks from {rebuild_queue} (timeout: 30s)...")
element = await fix_await(redis.brpop([rebuild_queue], timeout=30))
if element is None:
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue.")

Comment on lines +1195 to +1205
redis_logger.info(f"Connected to Redis, max retries set to {max_retries}")

while True:
logger.info("Waiting for tasks from triage_queue (timeout: 30s)...")
redis_logger.info("Waiting for tasks from triage_queue (timeout: 30s)...")
element = await fix_await(redis.brpop([RedisQueues.TRIAGE_QUEUE.value], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue

_, payload = element
logger.info("Received task from queue")
redis_logger.info("Received task from queue")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The Connected to Redis... and Received task from queue log messages are not highly recurrent or spammy. The connection log occurs only once at startup, and the task receipt log occurs only when a task is actually dispatched. Keeping these on the main logger is highly beneficial for observability and debugging in Sentry (e.g., as breadcrumbs leading up to an error), whereas only the idle polling logs (Waiting for tasks... and No tasks received...) should be ignored.

Suggested change
redis_logger.info(f"Connected to Redis, max retries set to {max_retries}")
while True:
logger.info("Waiting for tasks from triage_queue (timeout: 30s)...")
redis_logger.info("Waiting for tasks from triage_queue (timeout: 30s)...")
element = await fix_await(redis.brpop([RedisQueues.TRIAGE_QUEUE.value], timeout=30))
if element is None:
logger.info("No tasks received, continuing to wait...")
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue")
redis_logger.info("Received task from queue")
logger.info(f"Connected to Redis, max retries set to {max_retries}")
while True:
redis_logger.info("Waiting for tasks from triage_queue (timeout: 30s)...")
element = await fix_await(redis.brpop([RedisQueues.TRIAGE_QUEUE.value], timeout=30))
if element is None:
redis_logger.info("No tasks received, continuing to wait...")
continue
_, payload = element
logger.info("Received task from queue")

@mfocko
Copy link
Copy Markdown
Member Author

mfocko commented Jun 3, 2026

As for the Gemini review:

  1. I don't think that Sentry is the no.1 choice when debugging the running deployment, the log messages are still present within the pod (and potentially Sumo/Splunk, whatever the current “best-solution-on-market” choice is).
  2. Even if, I don't really see any added benefit in Received task from queue with regards to debugging/troubleshooting.

@mfocko
Copy link
Copy Markdown
Member Author

mfocko commented Jun 3, 2026

  1. Even if, I don't really see any added benefit in Received task from queue with regards to debugging/troubleshooting.

actually, now that I think about it… Sentry allows also collection of metrics, so maybe it would be better to track the proportions of the tasks (received, retried, success, failed), instead of pushing the repeating log message to logs?

@mfocko mfocko merged commit 7f3366c into packit:main Jun 4, 2026
9 checks passed
@mfocko mfocko deleted the feat/sentry/factor-out-redis-logging branch June 4, 2026 11:47
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