Skip to content

Conversation

@asnare
Copy link
Contributor

@asnare asnare commented May 15, 2025

This PR provides test coverage of the custom logger that blueprint installs, and establishes the baseline behaviour. Changes include:

  • Type-hints for the logger module, and updated docstrings.
  • Plumbing to support testing of the logger configuration that we provide.
  • A tweak to the stream that is probed, if we're going to probe: it should be stderr, not stdout. (Currently the probing facility is unused, I believe). Closes Logging: enable color-support based on the logging stream rather than stdout #227.
  • Unit tests for the install_logger() method to verify the configuration that it sets up.
  • Unit tests for the custom formatter: NiceFormatter
    Some of the tests currently fail due to problems with the existing implementation that will be fixed in subsequent PRs.

All updates to the external interfaces are API-compatible.

Relates: #229, #230.

asnare added 8 commits May 15, 2025 14:41
In addition, pass through stderr as the stream for the nice formatter to probe because that's where output goes, and it might be different to stdout.
…we install.

This includes several tests marked as expected failures due to issues in the current implementation of the formatter.
@asnare asnare self-assigned this May 15, 2025
@asnare asnare requested a review from nfx as a code owner May 15, 2025 13:04
@asnare asnare added documentation Improvements or additions to documentation internal do not show this PR in changelog labels May 15, 2025
@asnare asnare requested review from a team and sundarshankar89 and removed request for nfx May 15, 2025 13:05
@github-actions
Copy link

github-actions bot commented May 15, 2025

✅ 40/40 passed, 2 skipped, 1m21s total

Running from acceptance #270

Copy link
Collaborator

@sundarshankar89 sundarshankar89 left a comment

Choose a reason for hiding this comment

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

LGTM

@asnare asnare merged commit 72523e8 into main May 15, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Ready for Review to Done in UCX May 15, 2025
@asnare asnare deleted the logger-tests branch May 15, 2025 17:05
asnare added a commit that referenced this pull request May 15, 2025
#235)

This PR updates the log formatter so that when non-colorised logs are
being produced, we the timestamps include second-granular timestamps.
Previously only the minute was logged, which aside from being
inconsistent with the colorised output is insufficient for a logged
timestamp.

This PR is stacked on top of #232.
asnare added a commit that referenced this pull request May 15, 2025
This PR fixes an issue with the colorised log formatter whereby it
wasn't properly formatting log entries if they contained `%`-style
placeholders (with arguments). Although we don't generally use this
style in our own projects, 3rd-party code does and if we don't support
this then logging from those components is incorrect.

This is stacked on top of #232 and resolves #230.
ericvergnaud added a commit that referenced this pull request May 15, 2025
* main:
  Fix argument interpolation in colorised logs (#233)
  Ensure the non-colorized logs also include timestamps with second-lev… (#235)
  Test coverage for the customised logger setup (#232)
  Type hints for existing installation tests (#231)

# Conflicts:
#	tests/unit/test_installation.py
asnare added a commit that referenced this pull request May 16, 2025
…236)

When colorising logger output, the logger name (conventionally the
module) is abbreviated: this abbreviating throws an exception during
formatting if the logger name contains `..`. This is unlikely to occur
during normal use, but may occur if the logger is used in a script that
was referred to via a path that includes `..`.

This PR is stacked on top of #232.
asnare added a commit that referenced this pull request May 16, 2025
This PR updates the names we use for the `WARNING` and `CRITICAL`
log-levels when produced colorised logs. Prior to this PR:

- When not-colorised, we used the conventional `WARNING` and `CRITICAL`
names.
 - When colorised, we used `WARN` and `FATAL` instead.

Although the latter is more compact, it's more important to: a) be
consistent; b) conform to the norms of the Python ecosystem and use the
conventional names for the logging levels.

This PR is stacked on top of #232 and resolves #229.
asnare added a commit that referenced this pull request May 16, 2025
This PR updates the format of logs to be consistent when exception
information is included, irrespective of whether the logs are colorised
or not. Prior to this PR, colorised logs were formatted slightly
differently to the Python norms:
```
16:06:10 ERROR [d.l.module] An error occurred: Traceback (most recent call last):
  File "/path/to/file.py", line 410, in some_function
    raise RuntimeError("Test exception.")
RuntimeError: Test exception.
```
(Note that the traceback starts on the line of the error.)

This PR updates this to be:
```
16:08:18 ERROR [d.l.module] An error occurred
Traceback (most recent call last):
  File "/path/to/file.py", line 410, in some_function
    raise RuntimeError("Test exception.")
RuntimeError: Test exception.
```

This PR is stacked on top of #232.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation internal do not show this PR in changelog

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants