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

Feature request: add "log" endpoint #65

Closed
BeyondVertical opened this issue Oct 12, 2022 · 13 comments
Closed

Feature request: add "log" endpoint #65

BeyondVertical opened this issue Oct 12, 2022 · 13 comments

Comments

@BeyondVertical
Copy link

Hello,
I just found out over at the Healthchecks repo, that there is a way of not getting mails every time a job fails.
If we use the "log" endpoint, then we can be flexible about the times a job can fail, before the mails are sent. See this post for the explanation how it would work: healthchecks/healthchecks#525 (comment).
Maybe you can add an option (example: "no-error-ping=true" => sends to "log" endpoint) to send "log" when the job fails, instead of the normal exit code.
To have that feature would be awesome. Thanks for looking into it and for runitor! :)

@cuu508
Copy link
Contributor

cuu508 commented Oct 12, 2022

For the usage pattern discussed in the linked issue, we'd want a way to tell runitor "when you're about to send a /fail signal, please send /log instead".

bdd added a commit that referenced this issue Oct 12, 2022
Healthchecks added a "log" ping type to cater to use cases where a
periodic command is likely to exit with failure status but expected to
eventually succeed in future instantiations.
healthchecks/healthchecks#525 (comment)

Add `-log-only-non-zero` to send a log ping instead of a failure when the
command exits with a non-zero status. If runitor fails to execute the
command (e.g. not found, no permission, system ran out of pids, ...) a
failure ping is still sent.

Addresses #65
@bdd
Copy link
Owner

bdd commented Oct 12, 2022

@blackscreen
Give the log-only-non-zero branch a try.

My manual testing:

%  ./build/runitor -slug gh-test -log-only-non-zero -- bash -c 'echo "i succeeded"'
%  ./build/runitor -slug gh-test -log-only-non-zero -- bash -c 'echo "i failed" >&2; exit 1'
%  ./build/runitor -slug gh-test -log-only-non-zero -- /nonexistent

yielded:
hc-logs

@cuu508 I'd appreciate a quick code review.

@BeyondVertical
Copy link
Author

@blackscreen Give the log-only-non-zero branch a try.

The screenshot looks promising. Is there a build for linux I could download? I am not that familiar with go and building it from scratch. Sorry.

@bdd
Copy link
Owner

bdd commented Oct 12, 2022

@blackscreen Give the log-only-non-zero branch a try.

The screenshot looks promising. Is there a build for linux I could download? I am not that familiar with go and building it from scratch. Sorry.

Alright. I made a beta release. Binaries for various operating systems and architectures are at https://github.com/bdd/runitor/releases/tag/v0.11.0-beta.1

@BeyondVertical
Copy link
Author

Alright. I made a beta release. Binaries for various operating systems and architectures are at https://github.com/bdd/runitor/releases/tag/v0.11.0-beta.1

Thanks. I tested it and it works like a charm.

@cuu508
Copy link
Contributor

cuu508 commented Oct 13, 2022

Thanks for looking into it so promptly @bdd! The code change looks good to me. I experimented with the beta build too, and it did everything as I was expecting.

If runitor fails to execute the command (e.g. not found, no permission, system ran out of pids, ...) a failure ping is still sent.

A subtle thing that I didn't think of but makes total sense!

-log-only-non-zero can be interpreted two ways:

If the command exits with a non-zero status then log the fact only (do not signal a failure)

or

When inspecting the command's exit status, send a log ping only if the status is non-zero, and send something else in all other cases

Neither interpretation is wrong, so not really a problem. But perhaps -log-non-zero would work too? It is shorter, and to me reads as "if the status is non-zero, then log it".

@bdd
Copy link
Owner

bdd commented Oct 13, 2022

When inspecting the command's exit status, send a log ping only if the status is non-zero, and send something else in all other cases

Neither interpretation is wrong, so not really a problem. But perhaps -log-non-zero would work too? It is shorter, and to me reads as "if the status is non-zero, then log it".

I spent an order of magnitude more time thinking what this flag should be than writing the code :)

I included "only" in flag's name thinking it'd relay the "log instead of concluding as failure" behavior. Given that it wasn't immediately obvious to you, it's fair to assume I wasn't very successful in naming this. The fact that logging as in sending the output of the command as part of status ping is the default for HC+runitor users, a different type of ping which is also called "log" makes this more challenging.

I plan to cut v0.11.0 release Friday afternoon (Pacific Time). So unless you come up with a different flag name, I'll adopt your recommendation and call it -log-nonzero (notice the drop of hyphenation in "non-zero". Merriam-Webster lists is
as a closed compound word https://www.merriam-webster.com/dictionary/nonzero)

@cuu508
Copy link
Contributor

cuu508 commented Oct 13, 2022

I think -log-nonzero would work.

A couple other ideas, no strong opinion, naming is hard, –

-tolerate-nonzero or -tolerate-nonzero-status – I think it is intuitive, except it by itself does not explain what "tolerating" means

-report-nonzero-as=log or -nonzero-mode=int|fail|log, where

  • int means to use /<uuid>/%d
  • fail means to use /<uuid>/fail
  • log means to use /<uuid>/log

@BeyondVertical
Copy link
Author

BeyondVertical commented Oct 14, 2022

-report-nonzero-as=log or -nonzero-mode=int|fail|log, where

  • int means to use /<uuid>/%d
  • fail means to use /<uuid>/fail
  • log means to use /<uuid>/log

That (report-nonzero-as) would be my choice, but I am fine with either one of the mentioned ideas. Naming is really hard, I know that. I am just glad it got implemented so fast. :)

bdd added a commit that referenced this issue Oct 15, 2022
Context:
Healthchecks added a "log" ping type to cater to use cases where a
periodic command is likely to exit with failure status but expected to
eventually succeed in future instantiations.
healthchecks/healthchecks#525 (comment)

Problems:
- "log" ping type is not supported.
- Let the user decide ping type to be sent for success, nonzero exit, or
  execution failure (e.g. command not found, no permission, system ran
  out pids, ...)

Changes:
- Introduce three new flags:
  + -on-success: ping type type to send when command exits successfully.
                 defaults to "success".
  + -on-nonzero-exit: ping type to send when command exits with nonzero code.
                      defaults to "exit-code".
  + -on-exec-fail: ping type to send when runitor cannot execute the command.
                   defaults to "fail"
  + valid values for these flags are: "exit-code", "success", "fail", "log".

- Addresses lack of "log" ping type support and use case in #65.
bdd added a commit that referenced this issue Oct 15, 2022
Context:
Healthchecks added a "log" ping type to cater to use cases where a
periodic command is likely to exit with failure status but expected to
eventually succeed in future instantiations.
healthchecks/healthchecks#525 (comment)

Problems:
- "log" ping type is not supported.
- Let the user decide ping type to be sent for success, nonzero exit, or
  execution failure (e.g. command not found, no permission, system ran
  out pids, ...)

Changes:
- Introduce three new flags:
  + -on-success: ping type type to send when command exits successfully.
                 defaults to "success".
  + -on-nonzero-exit: ping type to send when command exits with nonzero code.
                      defaults to "exit-code".
  + -on-exec-fail: ping type to send when runitor cannot execute the command.
                   defaults to "fail"
  + valid values for these flags are: "exit-code", "success", "fail", "log".

- Addresses lack of "log" ping type support and use case in #65.
@bdd
Copy link
Owner

bdd commented Oct 15, 2022

I added the ability to customize the ping type for success, non-zero exit from command, and execution failure cases.

Usage text of the three new flags:

runitor --help |& grep -A 1 -- -on
  -on-exec-fail value
        Ping type to send when runitor cannot execute the command (exit-code|success|fail|log (default fail))
  -on-nonzero-exit value
        Ping type to send when command exits with a nonzero code (exit-code|success|fail|log (default exit-code))
  -on-success value
        Ping type to send when command exits successfully (exit-code|success|fail|log (default success))

Example use for the subject use-case of this issue:

runitor [...] -on-nonzero-exit log -- /opt/bin/flakey

With great flexibility comes great opportunities to do cursed things. So something like this is also possible:

runitor [...] -on-success fail -on-nonzero-exit success -- bash -c "exit ((RANDOM % 2))"

¯\_(ツ)_/¯  

This change slightly modifies runitor's default reporting behavior. I believe in a good way.

  • Execution failure now reports "fail" instead of reporting exit status 1. Provides a subtle distinction between command's faults or runitor's troubles.
  • Successful execution now reports "success" instead of exit status 0. You get the same green "OK" on the web and no redundant information of "exit status 0".

There is a good chance I missed or broke something. So please test the beta.2 release.

@BeyondVertical
Copy link
Author

BeyondVertical commented Oct 15, 2022

I tried beta 2 (only the "on-nonzero-exit"-flag) and it works great. I think you found the best solution with the new flags. The cursed things could be good for testing. Great job!

bdd added a commit that referenced this issue Oct 16, 2022
Context:
Healthchecks added a "log" ping type to cater to use cases where a
periodic command is likely to exit with failure status but expected to
eventually succeed in future instantiations.
healthchecks/healthchecks#525 (comment)

Problems:
- "log" ping type is not supported.
- Let the user decide ping type to be sent for success, nonzero exit, or
  execution failure (e.g. command not found, no permission, system ran
  out pids, ...)

Changes:
- Introduce three new flags:
  + -on-success: ping type type to send when command exits successfully.
                 defaults to "success".
  + -on-nonzero-exit: ping type to send when command exits with nonzero code.
                      defaults to "exit-code".
  + -on-exec-fail: ping type to send when runitor cannot execute the command.
                   defaults to "fail"
  + valid values for these flags are: "exit-code", "success", "fail", "log".

- Addresses lack of "log" ping type support and use case in #65.
@cuu508
Copy link
Contributor

cuu508 commented Oct 17, 2022

I tested the new flags in various combinations, and in combinations with the other flags, and all looked good. I like the flag names you picked, not the shortest, but clear and descriptive.

@bdd
Copy link
Owner

bdd commented Oct 22, 2022

I released v1.0.0 🎉 including this change.

@bdd bdd closed this as completed Oct 22, 2022
GermanCoding pushed a commit to GermanCoding/runitor that referenced this issue Oct 23, 2022
Context:
Healthchecks added a "log" ping type to cater to use cases where a
periodic command is likely to exit with failure status but expected to
eventually succeed in future instantiations.
healthchecks/healthchecks#525 (comment)

Problems:
- "log" ping type is not supported.
- Let the user decide ping type to be sent for success, nonzero exit, or
  execution failure (e.g. command not found, no permission, system ran
  out pids, ...)

Changes:
- Introduce three new flags:
  + -on-success: ping type type to send when command exits successfully.
                 defaults to "success".
  + -on-nonzero-exit: ping type to send when command exits with nonzero code.
                      defaults to "exit-code".
  + -on-exec-fail: ping type to send when runitor cannot execute the command.
                   defaults to "fail"
  + valid values for these flags are: "exit-code", "success", "fail", "log".

- Addresses lack of "log" ping type support and use case in bdd#65.
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

No branches or pull requests

3 participants