Skip to content

[Bug]: Use a unique token for log pagination instead of a timestamp #2845

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

Merged
merged 1 commit into from
Jun 26, 2025

Conversation

peterschmidt85
Copy link
Contributor

Fixes #2832, #2833, and potentially #2783

Here's what this PR does:

[Major]

[Minor]

  • Fixed the potential OOM issue with File log storage
  • UI now is capable of showing up to 10000000 rows (previously was 1000)

Here's what this PR doesn't solve:

Comment on lines +94 to +96
except LogStorageError:
# Group doesn't exist, re-raise the LogStorageError
raise
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why catching and re-raising immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it looks a bit odd but the idea is to catch a abstract exception and re-raise only if indeed streams do not exist.
This allows to heck stream existence only if error occurs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You catch and re-raise LogStorageError, which is basically the same as not catching it in the first place.

Comment on lines -105 to -108
# We intentionally make reading logs slow to prevent hitting GCP quota.
# This doesn't help with many concurrent clients but
# should help with one client reading all logs sequentially.
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without it, you'd hit quota and wouldn't be able to read the logs at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Do you think we can raise a specific exception and handle it on the client?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We already return an error when the quota is hit, see above. At that point, the client can do very little – you may have to wait for a minute or so. So we prevent quota hit in the first place.

@peterschmidt85 peterschmidt85 merged commit b2ef2d7 into master Jun 26, 2025
25 checks passed
@peterschmidt85 peterschmidt85 deleted the 2833-logs-token-pagination branch June 26, 2025 09:55
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.

[Bug]: Use a unique token for log pagination instead of a timestamp [Bug]: Can't see the entire log of a run via UI
2 participants