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

cleaned up logging output #797

Merged
merged 12 commits into from Jul 14, 2022
Merged

Conversation

PietroPasotti
Copy link
Contributor

@PietroPasotti PietroPasotti commented Jul 11, 2022

Fixes #796

Got rid of a few 'unhelpful' debug logger calls.

Checklist

  • [n/a] Have any types changed? If so, have the type annotations been updated?
  • [n/a] If this code exposes model data, does it do so thoughtfully, in a way that aids understanding?
  • Do error messages, if any, present charm authors or operators with enough information to solve the problem?

QA steps

  • I ensured that all 'exceptional' code paths still result into SOME logging output, while the 'normal' behaviour is more silent.
  • The logging output was used in unittests to check that a given branch was being executed; I removed those checks but we might want to replace them with other types of checks, so as not to lose coverage.
  • Is there any chance that 3rd party code is also relying on our logging output to do things? Are log streams considered 'private'?

Changelog

  • Cleaned up ops.main logging output.

ops/main.py Show resolved Hide resolved
Copy link

@marcoppenheimer marcoppenheimer left a comment

Choose a reason for hiding this comment

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

This looks great, thank you!
Although possibly out of scope, the Operator Framework 1.5.0 up and running. logs have limited use/interest for the user. Would we ever expect this not to be the case? If it wasn't, would a user be able be reasonably expected to do anything about it?

That said, Penny's comment on them makes sense, might we change the output to something richer? I don't have strong opinions on this though.

Inspiration

  • <CHARM_NAME> run started with <X> <Y> <Z> env vars - X Y Z being interesting ones.
  • ------------- <UNIT NAME> started ------------- - Just for demarcation of runs when using juju debug-log --include-module unit.<app>/N or something. Kinda ugly though.

@PietroPasotti
Copy link
Contributor Author

This looks great, thank you! Although possibly out of scope, the Operator Framework 1.5.0 up and running. logs have limited use/interest for the user. Would we ever expect this not to be the case? If it wasn't, would a user be able be reasonably expected to do anything about it?

That said, Penny's comment on them makes sense, might we change the output to something richer? I don't have strong opinions on this though.

Inspiration

  • <CHARM_NAME> run started with <X> <Y> <Z> env vars - X Y Z being interesting ones.
  • ------------- <UNIT NAME> started ------------- - Just for demarcation of runs when using juju debug-log --include-module unit.<app>/N or something. Kinda ugly though.

I wouldn't mind at all having a broader 'welcome' log on start/install, or anyway when the OF fires up for the first time.
We don't need to be reminded of the OF version on each hook call, do we? But as @pengale said it is handy to know.

We could specialize log output:

  • when the first hook ever runs on this unit: log "Operator Framework [VERSION] running charm [CHARM NAME]." The unit name prefix tag is added by juju so we don't need to worry about that.
  • on any hook run, simply say (as we already do I believe) "firing event [EVENT NAME]"

Copy link
Contributor

@pengale pengale left a comment

Choose a reason for hiding this comment

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

(A richer log message on start also makes sense, though possibly part of a separate PR.)

Copy link
Contributor

@rwcarlsen rwcarlsen left a comment

Choose a reason for hiding this comment

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

Just one small recommendation.

ops/main.py Show resolved Hide resolved
@PietroPasotti PietroPasotti merged commit 0a09774 into canonical:main Jul 14, 2022
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.

Temper the chattiness of DEBUG logs
5 participants