engine: reduce connection retry noise #5918
Merged
Conversation
This file contains 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
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Oct 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io> Signed-off-by: Christian Schlatter <schlatter@puzzle.ch>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Nov 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io> Signed-off-by: Christian Schlatter <schlatter@puzzle.ch>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Nov 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Nov 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Nov 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io>
schlapzz
pushed a commit
to schlapzz/dagger
that referenced
this pull request
Nov 24, 2023
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup. This creates a lot of scary-looking but harmless logs in the common case. This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging. Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far. One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the `session` subcommand, so scoping that out for now to get this fix in quick. Signed-off-by: Erik Sipsma <erik@dagger.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The engine client initializes its session w/ the server in a separate goroutine (due to the fact that it blocks for the duration of the session). This causes initial connection attempts to usually fail while the session is still be setup.
This creates a lot of scary-looking but harmless logs in the common case.
This changes the logic to only start printing failed connection attempts once the retry logic is going to wait over a second before the next attempt. The idea is to balance not showing useless errors but also not leave the user hanging for too long with no output and not hide errors that may useful for debugging.
Would love to have a test for this, but very hard to test timing things w/out flakes, so just manually tested so far.
One potentially better solution would be to have a debug mode and always hide errors unless that's enabled, but that would likely impact every SDK because they all use the CLI via the
sessionsubcommand, so scoping that out for now to get this fix in quick.cc @kpenfound
Fixes #5911