Skip to content

Log stack traces only in debug mode #12989

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sannya-singal
Copy link
Contributor

@sannya-singal sannya-singal commented Aug 11, 2025

Motivation

As a part of the initiative for improving Localstack error messages and stack traces, this PR aims to ensure that stack traces are logged when debug mode is enabled. Currently, in some use cases stack traces are logged without checking if debug mode is enabled or not. This causes excessive log noise even when DEBUG=0.

Changes

This PR cleans up logging across different use cases to ensure that stack traces are only logged when debug mode is enabled.

@sannya-singal sannya-singal self-assigned this Aug 11, 2025
@sannya-singal sannya-singal added the semver: patch Non-breaking changes which can be included in patch releases label Aug 11, 2025
@sannya-singal sannya-singal added this to the 4.8 milestone Aug 11, 2025
Copy link

github-actions bot commented Aug 11, 2025

S3 Image Test Results (AMD64 / ARM64)

    2 files  ±0    2 suites  ±0   8m 56s ⏱️ +23s
  515 tests ±0  465 ✅ ±0   50 💤 ±0  0 ❌ ±0 
1 030 runs  ±0  930 ✅ ±0  100 💤 ±0  0 ❌ ±0 

Results for commit 85890e1. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 11, 2025

Test Results - Preflight, Unit

22 063 tests  ±0   20 329 ✅ ±0   6m 26s ⏱️ +5s
     1 suites ±0    1 734 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 85890e1. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 11, 2025

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 19s ⏱️ +12s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 85890e1. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 11, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 21m 9s ⏱️
4 984 tests 4 393 ✅ 591 💤 0 ❌
4 990 runs  4 393 ✅ 597 💤 0 ❌

Results for commit 85890e1.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Aug 11, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 42m 51s ⏱️ +10s
4 625 tests ±0  4 186 ✅ ±0  439 💤 ±0  0 ❌ ±0 
4 627 runs  ±0  4 186 ✅ ±0  441 💤 ±0  0 ❌ ±0 

Results for commit 85890e1. ± Comparison against base commit cd7b481.

♻️ This comment has been updated with latest results.

Comment on lines 406 to 410
LOG.exception(
"Could not validate the notification destination %s",
target_arn,
exc_info=LOG.isEnabledFor(logging.DEBUG),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this stayed LOG.exception

This is very nice, would it be possible to do the following so that we still get why it failed? Sorry it's on me, the current exception logging wasn't that great for this one:

except ClientError as e:
    code = response["Error"]["Code"]
    LOG.error(
        "Could not validate the notification destination %s: %s",
        target_arn,
        code,
        exc_info=LOG.isEnabledFor(logging.DEBUG),
    )

So that the user would still get an indication as to why it failed, as this is a Boto error, we can provider at least the name of the exception like "AccessDenied" for example.

I had started reviewing the rest but saw you put back on draft 👍 will do the rest later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super sorry for the early ping 🙏 There are more incoming changes. Sure 💯 I will fix this one 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants