Skip to content

Conversation

@ayirr7
Copy link
Contributor

@ayirr7 ayirr7 commented Apr 3, 2025

We currently don't have these logs, which will also be helpful for breaking down per-consumer

@ayirr7 ayirr7 requested review from a team as code owners April 3, 2025 20:31
@ayirr7 ayirr7 requested a review from fpacifici April 3, 2025 22:24

METRICS_FREQUENCY_SEC = 1.0 # In seconds
BACKPRESSURE_THRESHOLD = 5.0 # In seconds
LOGGING_FREQUENCY_SEC = 60.0 # In seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it longer now that we know what the likely issue is.
64 consumers would log this every second.
What about once every 2-3 minutes ?

def _run_once(self) -> None:
self.__metrics_buffer.incr_counter("arroyo.consumer.run.count", 1)

if time.time() - self.__last_run_log_ts >= LOGGING_FREQUENCY_SEC:
Copy link
Member

Choose a reason for hiding this comment

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

please only calculate time.time() once, it's already very expensive to run it per _run_once

we also already have a counter metric for this, what is the purpose of the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per-consumer breakdown

)

self.__last_run_log_ts = time.time() # This is for throttling the logging of each run loop per-consumer
self.__last_pause_ts = None
Copy link
Member

Choose a reason for hiding this comment

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

you can simplify your logic by setting all of these to 0, then you don't have to check for None


# Records a log if the consumer has been active but receiving no message from poll() for longer than a threshold duration
elif time.time() - self.__last_empty_msg_ts >= LOGGING_FREQUENCY_SEC:
logger.info(f"Consumer is not paused but did not receive a message from underlying consumer for {LOGGING_FREQUENCY_SEC} seconds")
Copy link
Member

Choose a reason for hiding this comment

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

seems severe enough to warn

@untitaker untitaker mentioned this pull request Apr 8, 2025
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