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

fix: Improve error handling when monitored appliance is not available #30

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Apr 15, 2022

Dear Frederic,

this patch aims to improve robustness of non-happy code paths after bringing in #19 the other day.

Previously, the program yielded the rather cryptic error message <built-in function get> returned NULL without setting an error and also added a traceback, not catching the exception.

Now, any such exceptions are caught and cleanly converged into the UNKNOWN state, so that the program croaks with:

UNKNOWN - timed out while connecting to remote host

Note: The improved error message will only be available when installing the latest easysnmp from the upstream repository.

With kind regards,
Andreas.

Before

How it looked like on my aNag when the appliance went down.

image

After

Now it looks compact and clean again.

image

With all six sensors active, the space-saving is even more dramatic and obvious. And of course, the error message is way clearer than before.

image

@amotl
Copy link
Contributor Author

amotl commented Apr 15, 2022

Note: The improved error message timed out while connecting to remote host will only be available when installing the latest easysnmp package from its upstream repository.

I just asked @kamakazikamikaze about the state of affairs over at easysnmp/easysnmp#147.

@kamakazikamikaze
Copy link

Please let me know if a full release instead of a pre-release is required. I was hoping to get a few more bugfixes out for 0.2.6 but it's more important to me that projects dependent on the enhancements are able to operate effectively.

@amotl
Copy link
Contributor Author

amotl commented Apr 21, 2022

Dear Kent,

thank you very much for your quick response on this matter. I think it is no problem to install both packages using an incantation like

pip install git+https://github.com/wernerfred/check_synology easysnmp==0.2.6a1

until 0.2.6 GA will be published 1.

It's more important to me that projects dependent on the enhancements are able to operate effectively.

I very much appreciate that, keep up the spirit. Good luck with the next round of fixes to easysnmp!

With kind regards,
Andreas.

Footnotes

  1. At the same time, this reminds us that @wernerfred might also decide to publish this package to PyPI within one of the next iterations?

@wernerfred
Copy link
Owner

wernerfred commented Apr 22, 2022

I think it is no problem to install both packages using an incantation like

pip install git+https://github.com/wernerfred/check_synology easysnmp==0.2.6a1

Should we add a hint on this specific version in the README?

  1. At the same time, this reminds us that @wernerfred might also decide to publish this package to PyPI within one of the next iterations?

No real objections on it but also never did this and low on time. I would favor a solution that is build on github actions that will release/update pipy alongside github releases (or is this a onetimer and will automatically pick up new releases from github?)

EDIT: there is one already in place and explained on official site: https://packaging.python.org/en/latest/guides/publishing-package-distribution-releases-using-github-actions-ci-cd-workflows/

@wernerfred wernerfred linked an issue Apr 22, 2022 that may be closed by this pull request
@amotl
Copy link
Contributor Author

amotl commented Jul 12, 2022

Dear Kent (@kamakazikamikaze),

we can confirm easysnmp-0.2.6a1 works very well for us.

Please let me know if a full release instead of a pre-release is required.

Indeed, a full release would be needed in order to express it in the setup.py file as easysnmp>=0.2.6. I don't think it will work otherwise, unless the user specifies pip install --pre.

Do you see any chance to publish a full 0.2.6 release of easysnmp? I think the community will be really grateful.

With kind regards,
Andreas.

@amotl
Copy link
Contributor Author

amotl commented Jul 12, 2022

Dear Werner,

Should we add a hint on this specific version in the README?

I think all will be fine as soon as easysnmp-0.2.6 GA is published. When adjusting setup.py appropriately, we will not need any specific admonition in the README file.

What about publishing this package to PyPI?

I would favor a solution that is build on github actions that will release/update pipy alongside github releases.

I hear you. We have a diverse set of release procedures on our Python repositories, also based on GHA. When I can find some time, I will pick the best-of-breed solution and submit a corresponding patch here. Maybe next winter.

With kind regards,
Andreas.

@kamakazikamikaze
Copy link

I plan to publish 0.2.6 this weekend. I finally got around to addressing an SNMP v3 issue while sneaking in some proper Net-SNMP library cleanup. Just know that I will likely be moving the repo to the new org I created earlier this year.

@kamakazikamikaze
Copy link

I fast-tracked the publication. easysnmp-0.2.6 is now available via pip.

Copy link
Owner

@wernerfred wernerfred left a comment

Choose a reason for hiding this comment

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

Any other objections?

README.md Outdated
Comment on lines 29 to 36
While the program will work already, the most recent official release of the
``easysnmp`` package available on PyPI is from Jun 15, 2017. So, unless a new
version gets published, we recommend to use the latest ``easysnmp`` from the
upstream repository. It received many improvements just recently (2022).
```shell
pip install git+https://github.com/kamakazikamikaze/easysnmp
```

Copy link
Owner

Choose a reason for hiding this comment

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

Well - we can remove this then, can't we?

Copy link
Contributor Author

@amotl amotl Aug 1, 2022

Choose a reason for hiding this comment

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

Dear Frederic,

thank you. I've just refreshed the patch and removed this section. Instead, I've added "easysnmp>=0.2.6,<1" as dependency constraint.

Also thank you very much, @kamakazikamikaze, for releasing easysnmp-0.2.6!

With kind regards,
Andreas.

@wernerfred
Copy link
Owner

wernerfred commented Jul 20, 2022

I fast-tracked the publication. easysnmp-0.2.6 is now available via pip.

@all-contributors pls add @kamakazikamikaze for plugin

Thx for your support here (regarding easysnmp) and quick reactions!

@allcontributors
Copy link
Contributor

@wernerfred

I've put up a pull request to add @kamakazikamikaze! 🎉

Previously, the program yielded the rather cryptic error message
`<built-in function get> returned NULL without setting an error` and
also added a traceback, not catching the exception.

Now, any such exceptions are caught and cleanly converged into the
`UNKNOWN` state, so that the program croaks with:

    UNKNOWN - timed out while connecting to remote host

The improved error handling will only be available with easysnmp>=0.2.6.
@wernerfred wernerfred merged commit 73da7d8 into wernerfred:master Aug 2, 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.

SystemError: returned NULL without setting an error
3 participants