Skip to content

Conversation

@agrasth
Copy link
Contributor

@agrasth agrasth commented May 28, 2025

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • This pull request is on the dev branch.
  • I used gofmt for formatting the code before submitting the pull request.

Improve RetryExecutor Logging with Enhanced Context and Clarity

🎯 Overview

Enhanced the RetryExecutor.LogRetry() function to provide more informative and contextual logging for better debugging and monitoring of retry operations.

Changes Made

Enhanced Log Message Structure

  • Clear attempt labeling: [Initial attempt] vs [Retry X/Y] for better progress tracking
  • Consistent formatting: Improved message structure with clear error separation
  • Contextual information: More descriptive messages for different scenarios

Improved Log Levels

  • ERROR: Final failed attempts (critical issues)
  • WARN: Retry attempts with errors (expected during retries)
  • INFO: Successful operations after retries (important milestones)
  • DEBUG: First-try successes (routine operations)

Before:

[Warn] (Attempt 1) - Failure occurred while sending POST request to https://ftdr.jfrog.io/artifactory/api/search/aql: Post " https://ftdr.jfrog.io/artifactory/api/search/aql": dial tcp 34.74.126.177:443: i/o timeout

After:

WARN: [Retry 1/5] network timeout - Error: timeout error
WARN: [Retry 2/5] network timeout - Error: timeout error
INFO: [Retry 3/5] network timeout - Succeeded after 3 retries

Comment on lines 91 to 114
errStr := err.Error()
switch {
case strings.Contains(errStr, "i/o timeout"):
message = fmt.Sprintf("%s: Temporary network timeout occurred, retrying...", message)
case strings.Contains(errStr, "connection refused"):
message = fmt.Sprintf("%s: Connection refused - server may be temporarily unavailable, retrying...", message)
case strings.Contains(errStr, "no such host"):
message = fmt.Sprintf("%s: DNS resolution failed - host may be temporarily unavailable, retrying...", message)
case strings.Contains(errStr, "connection reset"):
message = fmt.Sprintf("%s: Connection was reset - network may be unstable, retrying...", message)
case strings.Contains(errStr, "TLS handshake timeout"):
message = fmt.Sprintf("%s: TLS handshake timeout - server may be busy, retrying...", message)
case strings.Contains(errStr, "too many open files"):
message = fmt.Sprintf("%s: System file limit reached - waiting for resources to be freed, retrying...", message)
case strings.Contains(errStr, "503"):
message = fmt.Sprintf("%s: Service temporarily unavailable (503) - server is overloaded, retrying...", message)
case strings.Contains(errStr, "502"):
message = fmt.Sprintf("%s: Bad gateway (502) - upstream server may be unavailable, retrying...", message)
case strings.Contains(errStr, "504"):
message = fmt.Sprintf("%s: Gateway timeout (504) - upstream server is not responding, retrying...", message)
case strings.Contains(errStr, "429"):
message = fmt.Sprintf("%s: Too many requests (429) - rate limit exceeded, retrying...", message)
default:
message = fmt.Sprintf("%s: %s", message, errStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this specific to yarn? or artifactory? why is this change required?

@agrasth agrasth force-pushed the feature/rewording-yarn-logs branch from aa09dfe to bd34576 Compare May 29, 2025 20:01
@agrasth agrasth force-pushed the feature/rewording-yarn-logs branch from bd34576 to 08399ef Compare May 29, 2025 20:11
@agrasth agrasth requested a review from bhanurp May 29, 2025 20:12
@agrasth agrasth added the safe to test Approve running integration tests on a pull request label Jun 25, 2025
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Jun 25, 2025
if attemptNumber == 0 {
log.Debug(fmt.Sprintf("%s - Succeeded on first try", message))
} else {
log.Info(fmt.Sprintf("%s - Succeeded after %d retries", message, attemptNumber))
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be debug as well

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.

2 participants