Improve apps logs streaming helpers #3908
Open
+1,661
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ericfeunekes
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
pkosiec
reviewed
Nov 12, 2025
Member
pkosiec
left a comment
pkosiec
left a comment
There was a problem hiding this comment.
Thank you @ericfeunekes for your contribution! The new app logs command is super helpful. I tested the changes and it works well 👍
Before merge, please take a look at my comments. I can help you implementing the changes if you'd like to - feel free to ping me anytime. Thanks!
added 21 commits
November 13, 2025 15:25
This reverts commit 402e72743d9a9bd6dbdc7c24f1f20d62cededf11.
Author
|
@pkosiec I think I've addressed each of your issues and tried to keep them on separate commits to make it easier to review |
pkosiec
reviewed
Nov 14, 2025
Member
pkosiec
left a comment
pkosiec
left a comment
There was a problem hiding this comment.
Thanks a lot for implementing the comments, including the logs color formatting!
We're almost there - I put some minor comments. There's one small issue with -f and -tail-lines flags, but other than that I don't see any major blockers 👍
pkosiec
reviewed
Nov 17, 2025
pietern
reviewed
Nov 17, 2025
Contributor
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
Hi @ericfeunekes, thanks for the PR!
The token changes (cmd/auth/token_* and libs/auth/token_*) are not necessary. You can use the TokenSource function on the SDK configuration directly to get a fresh OAuth token. I confirmed that the following patch works: https://gist.github.com/pietern/41d02acc2907416244789c7408fb232f
Please move logstream to libs/apps to make it clearer that it is specific to apps.
Contributor
|
Btw, when I stop an app during tail, the logs command doesn't stop or error. |
Contributor
|
@ericfeunekes I forgot an important detail in the previous message. Contributing to CLI requires a signed CLA, if that's something you're willing to do, please reach out with a request to sign CLA to [email protected] and we will take it from there. Thanks! |
Replace custom token acquisition logic with SDK's built-in TokenSource function as suggested by @pietern. This simplifies the implementation by delegating token lifecycle management to the SDK configuration. Changes: - Use cfg.GetTokenSource() instead of auth.AcquireToken() - Remove custom token_loader abstraction (libs/auth/token_loader.go) - Revert cmd/auth/token.go to original implementation - Remove tokenAcquireTimeout constant (handled by SDK) This addresses review feedback from @pietern on Nov 17, 2025. Reference: https://gist.github.com/pietern/41d02acc2907416244789c7408fb232f 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Relocate logstream from libs/logstream to libs/apps/logstream to make it clearer that this package is specific to Databricks Apps functionality. Changes: - Move libs/logstream/streamer.go to libs/apps/logstream/streamer.go - Move libs/logstream/streamer_test.go to libs/apps/logstream/streamer_test.go - Update import in cmd/workspace/apps/logs.go This addresses review feedback from @pietern on Nov 17, 2025. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Group individual const declarations into a single const block for better code organization and readability. Changes: - Convert six individual const declarations to const block - Apply alignment formatting This addresses review feedback from @pkosiec on Nov 14, 2025. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When using --tail-lines with -f (follow), the buffer was flushing too early, showing all historical logs instead of just the last N lines. The tail buffer maintains a rolling window of the last N lines, but it was flushing as soon as it reached N lines when following, rather than waiting for the prefetch window to collect all historical logs first. Fix: Always respect the flush deadline (prefetch window) regardless of follow mode. This allows historical logs to stream in and the rolling buffer to maintain only the truly last N lines before flushing. Example with --tail-lines 10 -f: - Before: Shows lines 1-10, then 11-1000+ (all logs) - After: Shows lines 991-1000, then new logs (correct) This addresses review feedback from @pkosiec on Nov 14, 2025. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace magic number (30 seconds) with defaultHandshakeTimeout constant for better code maintainability. Changes: - Add defaultHandshakeTimeout = 30 * time.Second to const block - Use constant in newLogStreamDialer instead of inline value This addresses review feedback from @pkosiec on Nov 12/14, 2025. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
When following app logs (--follow) and the app stops or is deleted, the logs command now detects this and exits gracefully instead of retrying indefinitely. Implementation: - Added AppStatusChecker callback to logstream.Config - Check app status before each reconnect attempt in follow mode - Also check when connection closes normally during follow - Exit with clear error message when app is stopped/deleting/error state This addresses @pietern's comment about the command not stopping when the app is stopped during tail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Contributor
|
@ericfeunekes Can you update the PR summary to match the current state of the PR? |
Collaborator
|
Commit: 06aece0
11 failing tests:
Top 30 slowest tests (at least 2 minutes):
|
Contributor
|
@ericfeunekes Thanks for signing the CLA. This PR can proceed. Can you update the PR summary to match the current state of the PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
databricks apps logs NAMEcommand, including tail/follow/search/source/output-file flags wired viacmdgroup, file mirroring with 0600 perms, and validation against apps that lack a public URL. (cmd/workspace/apps)libs/logstreamhelper with token refresh hooks, buffering, search/source filtering, structured error handling, context-driven deadlines, and a comprehensive unit suite so other commands can stream logs without bespoke WebSocket loops.databricks auth tokento reuseauth.AcquireTokenvia the newlibs/auth/token_loader.go, ensuring persistent auth options and timeouts stay centralized and fully covered by regression tests.Why
This is a quality of life addition that allows the CLI to tail logs to the CLI. It hooks directly into the token acquisition module so that the tokens don't need to be managed separately.
Tests