-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Use a continuation token to get logs in ecs #31824
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
Conversation
o-nikolas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this one, I did not like the increasingly wasted calls.
Did you do any manual tests to ensure all the logs are coming through like you expect?
😬 |
Co-authored-by: Niko Oliveira <onikolas@amazon.com>
tested live, works well. |
| elif continuation_token.value != response["nextForwardToken"]: | ||
| num_consecutive_empty_response = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this check after the check on not event_count means that even when we get the same token twice in a row, clearly indicating that there is nothing more to read, we still do NUM_CONSECUTIVE_EMPTY_RESPONSE_EXIT_THRESHOLD (=3) calls before returning.
Moving the check on token above saves 2 calls every time we get the correct termination condition from cloudwatch.
| skip: int = 0, | ||
| start_from_head: bool = True, | ||
| continuation_token: ContinuationToken = ContinuationToken(), | ||
| continuation_token: ContinuationToken | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with the previous version, I didn't realize that the "default object" is created only once. And since we modify it in the method, the default object is modified, and when this method is called again with the default parameters, instead of getting a clean object, we get the modified one from the last call.
The current implem browses logs from the start every time it tries to get new logs, which results in longer and longer fetching times, and also many calls that can potentially trigger rate limits.
There is a very simple way to avoid this, with the token returned by every request that allows to continue from where the last call left.