Skip to content
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

Fix bugs on clever logs and enable retry #658

Merged
merged 9 commits into from
Feb 7, 2024
Merged

Conversation

hsablonniere
Copy link
Member

@hsablonniere hsablonniere commented Jan 19, 2024

What is in this PR?

  • Update @clevercloud/client to 8.0.3 to get all the small bug fixes around retry and network failures
  • Enable auto retry on network failures (we forgot to enable it when we switched to the new logs API)
  • Improve open and error debug message (wording and colors)
  • Only print SSE errors as debug when verbose mode is enabled
  • Improve error message with EAI_AGAIN and ECONNRESET

I also took the opportunity to fix/refactor a few details in the code (see commits).

What to QA?

Download the preview binary and test clever logs:

  • Without time window
  • With just --since
  • With just --until
  • With both --since and --until
  • Try to cut your internet connection for more than 20 secs
  • Try to cut your internet connection for a few seconds (or switch between 2 network interfaces)
    • The logs stream should auto retry and resume from the last received log
  • Try with an without -v

You can also test clever restart or clever deploy with network failures.

This app is really helpful to test logs https://github.com/CleverCloud/clever-test-logs

@hsablonniere hsablonniere requested a review from a team as a code owner January 19, 2024 09:29
@hsablonniere hsablonniere self-assigned this Jan 19, 2024
@hsablonniere hsablonniere force-pushed the add-retry-on-new-logs branch from b1cc170 to 887284d Compare January 30, 2024 16:41
Copy link

🔎 A preview has been automatically published:

  • 🐧 linux 6907123b4e9e3514e8db524e01c4aee746d43197f4c9ba0c2d9a3842dd185cf6
  • 🍏 macos 92d73c29ada46c96fa92d551e3d5e2d08270417e24c7424968a4e8f211ab2d25
  • 🪟 win 1f731b3533684dddce39de5f6911917773fd9205e1ddaa1777ce2dc39deefb10

This preview will be deleted once this PR is closed.

@davlgd
Copy link
Contributor

davlgd commented Jan 30, 2024

I'm not sure of what is expected or not, but here is what I observe (remember, I'm under macOS):

If I cut Wi-Fi:

  • On Current CLI:
[ERROR] an error occured: undefined
[ERROR] Error: retry is not enabled: TypeError: fetch failed
  • On Preview CLI:
[ERROR] fetch failed

If I cut Wi-Fi for a short time and it gets back:

  • On Current CLI:
[ERROR] an error occured: undefined
  • On Preview CLI:
No error message

If I use --since:

  • On Current CLI:
No error message
  • On Preview CLI:
No error message

If I use only --until:

  • On Current CLI:
[ERROR] Error: retry is not enabled: TypeError: Cannot read properties of undefined (reading 'length')
  • On Preview CLI:
No error message

If I use --since and --until:

  • On Current CLI:
[ERROR] Error: retry is not enabled: TypeError: Cannot read properties of undefined (reading 'length')
  • On Preview CLI:
No error message

Verbose

  • On Current CLI:
clever logs --until 2024-01-30T20:01:20.880Z -v
[DEBUG] Loading app configuration from /Users/davlgd/Downloads/clever-tools-add-retry-on-new-logs_macos/clever-test-logs/.clever.json
Waiting for application logs…
[DEBUG] Load configuration from environment variables
[DEBUG] Load configuration from /Users/davlgd/.config/clever-cloud/clever-tools.json
[DEBUG] stream opened! {"appId":"app_15e2f264-3049-4753-8c85-39dcbfb4a87f","filter":null,"deploymentId":null}
[STACKTRACE]
Error: retry is not enabled: TypeError: Cannot read properties of undefined (reading 'length')
[/STACKTRACE]
[ERROR] Error: retry is not enabled: TypeError: Cannot read properties of undefined (reading 'length')
  • On Preview CLI:
❯ ../clever logs --until 2024-01-30T20:01:20.880Z -v
[DEBUG] Loading app configuration from /Users/davlgd/Downloads/clever-tools-add-retry-on-new-logs_macos/clever-test-logs/.clever.json
Waiting for application logs…
[DEBUG] Load configuration from environment variables
[DEBUG] Load configuration from /Users/davlgd/.config/clever-cloud/clever-tools.json
[DEBUG] Logs stream (open) {"appId":"app_15e2f264-3049-4753-8c85-39dcbfb4a87f","filter":null,"deploymentId":null}

@aurrelhebert
Copy link
Member

I run some local tests and everything seem to work as expected. Good job!

When the clever-tools were using the `request.superagent` from the clever-client,
`EAI_AGAIN` and `ECONNRESET` errors were caught, and message were improved.
When we moved to the common request.fetch, we lost improvment.
This commit adds back the improved messages but in the clever-tools code.
This option was available on the superagent implementation but not on
the fecth implementation.
The inner implementation of Logger.debug already does nothing if the CLEVER_VERBOSE is not enabled
@hsablonniere hsablonniere force-pushed the add-retry-on-new-logs branch 2 times, most recently from c6b13c4 to 18083ca Compare February 6, 2024 16:42
@davlgd
Copy link
Contributor

davlgd commented Feb 6, 2024

With the latest build/push, evrything is ok for me: the CLI waits for an internet connection if it's missing at clever logs launch or after a cut. After some retries, I get an error, but if the connection is back, logs stream starts as expected.

Copy link
Contributor

@miton18 miton18 left a comment

Choose a reason for hiding this comment

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

QA OK

@hsablonniere hsablonniere force-pushed the add-retry-on-new-logs branch from 18083ca to a4ec4b9 Compare February 7, 2024 14:31
@hsablonniere hsablonniere merged commit 0a2255d into master Feb 7, 2024
5 checks passed
@hsablonniere hsablonniere deleted the add-retry-on-new-logs branch February 7, 2024 15:21
Copy link

github-actions bot commented Feb 7, 2024

🔎 The preview has been automatically deleted.

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