Skip to content

fix: implement logging with slog and persistent config#604

Merged
sabaini merged 2 commits intocanonical:mainfrom
sabaini:bug/239
Aug 13, 2025
Merged

fix: implement logging with slog and persistent config#604
sabaini merged 2 commits intocanonical:mainfrom
sabaini:bug/239

Conversation

@sabaini
Copy link
Copy Markdown
Collaborator

@sabaini sabaini commented Aug 11, 2025

Description

This commit introduces a new logging system based on slog. It also separates backend from cli logging.

  • Add new logger package with slog-based implementation
  • Legacy level compatibility for API backwards compatibility
  • Use new logger instead of legacy lxd-based logger
  • Add CLI logger package for command-line utilities with debug/verbose flags
  • Add tests

Fixes #239

Type of change

Delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Clean code (code refactor, test updates; does not introduce functional changes)

How has this been tested?

Existing and new unit tests

Contributor checklist

Please check that you have:

  • self-reviewed the code in this PR
  • added code comments, particularly in less straightforward areas
  • added tests to verify effectiveness of this change

@sabaini sabaini force-pushed the bug/239 branch 3 times, most recently from fc17e9b to 3eadda9 Compare August 11, 2025 16:56
@sabaini sabaini requested a review from lmlg August 11, 2025 17:11
Comment thread microceph/api/ops_maintenance.go Outdated
Comment thread microceph/client/client_configs.go
Copy link
Copy Markdown
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

Given that the API mostly doesn't work at the moment, do you think it is worthwhile to switch the API endpoint from microceph/configs/log-level to ops/log-level. Suggesting because we do have an ops endpoint and we use it for things like replication etc.

@UtkarshBhatthere
Copy link
Copy Markdown
Contributor

@sabaini i did a quick review, will do another one today.

@sabaini
Copy link
Copy Markdown
Collaborator Author

sabaini commented Aug 12, 2025

Given that the API mostly doesn't work at the moment, do you think it is worthwhile to switch the API endpoint from microceph/configs/log-level to ops/log-level. Suggesting because we do have an ops endpoint and we use it for things like replication etc.

Even if it was buggy it might have been used. I would opt for API stability unless we have very pressing need. I'd say we clean the API with the tentacle rel

Copy link
Copy Markdown
Contributor

@UtkarshBhatthere UtkarshBhatthere left a comment

Choose a reason for hiding this comment

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

This change looks good to me, approving. We still have some lingering instances of the lxd logger which may be required to be ported to the new one. Adding this as a ticket for future will:

  1. Help track that as something that needs to be done
  2. Can be shared with maintenance mode maintainers as feedback.

This commit introduces a new logging system based on slog. It also
separates backend from cli logging.

- Add new logger package with slog-based implementation
- Legacy level compatibility for API backwards compatibility
- Use new logger instead of legacy lxd-based logger
- Add CLI logger package for command-line utilities with debug/verbose flags
- Add tests

Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
create a wrapper for lxd logger for usage in the ops_maintenance

Signed-off-by: Peter Sabaini <peter.sabaini@canonical.com>
@sabaini
Copy link
Copy Markdown
Collaborator Author

sabaini commented Aug 13, 2025

Thanks for the review @UtkarshBhatthere . As the ops_maintenance functions use LXD functionality we'll need conform to the LXD logger somewhere. Your comment got me thinking though -- handling this might be easier with an adapter that that takes our logger and adapts it to the lxd logging package. Wdyt?

Copy link
Copy Markdown
Contributor

@lmlg lmlg left a comment

Choose a reason for hiding this comment

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

lgtm, good stuff

@sabaini sabaini merged commit 5f545a9 into canonical:main Aug 13, 2025
21 checks passed
@sabaini sabaini deleted the bug/239 branch August 13, 2025 15:48
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.

microceph spams logs with debug messages

3 participants