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

feat: improve HTTP API logging, honor no-startup-message #1091

Merged
merged 5 commits into from
Nov 29, 2021

Conversation

jinnatar
Copy link
Contributor

They provide little useful information outside debugging and anything at
Info level is broadcast over shoutrrr.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #1091 (ac8ad56) into main (4a66a69) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1091   +/-   ##
=======================================
  Coverage   61.96%   61.96%           
=======================================
  Files          22       22           
  Lines        1438     1438           
=======================================
  Hits          891      891           
  Misses        463      463           
  Partials       84       84           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a66a69...ac8ad56. Read the comment docs.

@piksel
Copy link
Member

piksel commented Oct 15, 2021

I don't think it is that straight forward. It essentially replaces the startup message and when --debug isn't passed, there is no output from starting watchtower at all which might be confusing for some users. That being said, the startup message should probably be displayed, but incorporating whether the API is enabled. That way you could disable it as well using --no-startup-message.

Copy link
Member

@simskij simskij left a comment

Choose a reason for hiding this comment

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

I agree with @piksel on this.

@jinnatar
Copy link
Contributor Author

ACK, reworking it to use --no-startup-message

While the log message is useful in seeing the HTTP API is enabled and
functioning, in production installations it should be silenced to
prevent notifications going out on every startup with the API enabled.
@jinnatar
Copy link
Contributor Author

@piksel PTAL. I left the runHTTPServer logging as Debug since it's of questionable value to a user, and changed the Info level log to contain the listen port so it's more useful in indicating the API is functioning and where to find it.

Example run:

%> ./watchtower daemon --http-api-update --http-api-token foo --http-api-periodic-polls --no-startup-message
INFO[0001] Watchtower v0.0.0-unknown                     notify=no
INFO[0001] Using no notifications                        notify=no
INFO[0001] Only checking containers with name "daemon"   notify=no
INFO[0001] Scheduling first run: 2021-10-24 16:29:02 +0300 EEST  notify=no
INFO[0001] Note that the first check will be performed in 23 hours, 59 minutes, 59 seconds  notify=no
INFO[0001] The HTTP API is enabled at :8080.             notify=no

@jinnatar
Copy link
Contributor Author

jinnatar commented Oct 23, 2021

There is still a slight annoyance left where watchtower is very silent and unhelpful, but I believe this is a deeper issue than I can resolve here:

%> ./watchtower daemon --http-api-update --http-api-token foo 
<absolutely no output produced>

i.e. if the API mode is enabled but no periodic updates are enabled as is the default, startup messages aren't processed at all since the call is blocked. Probably they should still happen, but then the flow needs to be changed a bit and my Go isn't exactly that good to do it in this PR without undue risk to general stability. :-)

Edit: nvm, found a clean way to fix this issue at the same time.

To prevent these getting sent as notifications, use --no-startup-message
Previously the info was logged only if a single run was requested but
now also the default API mode has a zero length schedule.
@jinnatar jinnatar changed the title Downgrade HTTP API startup log messages to Debug Improve HTTP API logging, allow it to be disabled with --no-startup-message Oct 23, 2021
@jinnatar jinnatar requested a review from simskij October 23, 2021 15:15
Copy link
Member

@piksel piksel left a comment

Choose a reason for hiding this comment

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

Looks good, except for a few smaller things.

The API parts are messy and should be redone (ref #939). But this should be sufficient for now. Just take a look at the comments.

cmd/root.go Outdated Show resolved Hide resolved
pkg/api/api.go Outdated Show resolved Hide resolved
cmd/root.go Show resolved Hide resolved
@jinnatar jinnatar requested a review from piksel October 25, 2021 12:12
@jinnatar
Copy link
Contributor Author

I agree with @piksel on this.

I believe these have been addressed but GitHub still marks this comment as blocking merge for some reason.

@piksel piksel changed the title Improve HTTP API logging, allow it to be disabled with --no-startup-message feat: improve HTTP API logging, honor no-startup-message Nov 29, 2021
@piksel piksel merged commit e14cc29 into containrrr:main Nov 29, 2021
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.

3 participants