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

catch unhandled rejection / errors #1

Closed
cjroebuck opened this issue Dec 5, 2019 · 10 comments
Closed

catch unhandled rejection / errors #1

cjroebuck opened this issue Dec 5, 2019 · 10 comments
Labels
enhancement New feature or request released

Comments

@cjroebuck
Copy link

Hey thanks for this lib, very useful!

Occasionally I receive a 404 error from the kubernetes API, this is usually when a pod is starting up. This causes the promise in your lib to reject:

HTTPError: Response code 404 (Not Found)
    at EventEmitter.emitter.on (/usr/src/app/node_modules/preoom/node_modules/got/source/as-promise.js:74:19)
    at process._tickCallback (internal/process/next_tick.js:68:7)

podmetrics.metrics.k8s.io "default/my-test-pod" not found

Right now it seems I can't catch these errors in your lib as they're running in a different async context to the calling code so they only get caught by my unhandledRejection handler. Is there a way to handle this?

@gajus gajus added the bug Something isn't working label Dec 5, 2019
@gajus
Copy link
Owner

gajus commented Dec 6, 2019

What is the expected behaviour in this case?

@gajus
Copy link
Owner

gajus commented Dec 6, 2019

I would suggest to simply delay initialisation of preoom.

@gajus gajus added enhancement New feature or request and removed bug Something isn't working labels Dec 6, 2019
@cjroebuck
Copy link
Author

Hey there, it would be great if you could wrap a try/catch around your observe function and then pass any caught error back as the first arg of the callback.

That way the caller can decide how they want to handle the errors. Otherwise it will only be handled by an unhandled rejection handler which is not great.

I ran this in my cluster today and got a ton of 404 errors due to pods being scaled up /down and evicted etc so it seems to be fairly common to get these errors due to metrics not being ready in time

@gajus
Copy link
Owner

gajus commented Dec 7, 2019

Hey there, it would be great if you could wrap a try/catch around your observe function and then pass any caught error back as the first arg of the callback.

The callback is only called once, though. It would need to be an event handler.

@gajus
Copy link
Owner

gajus commented Dec 7, 2019

I ran this in my cluster today and got a ton of 404 errors due to pods being scaled up /down and evicted etc so it seems to be fairly common to get these errors due to metrics not being ready in time

So how would you handle this case that you are describing?

@cjroebuck
Copy link
Author

cjroebuck commented Dec 8, 2019 via email

@cjroebuck
Copy link
Author

Basically, wrap a try/catch round here[1]. It will catch any errors from the await getPodResourceUsage() call and pass them back in the callback.

  1. const context = await ready;
    const podResourceSpecification = context.podResourceSpecification;
    const podResourceUsage = await getPodResourceUsage();
    log.debug({
    podResourceSpecification,
    podResourceUsage
    }, 'observed resource usage');

@gajus gajus closed this as completed in bb07437 Dec 12, 2019
@gajus
Copy link
Owner

gajus commented Dec 12, 2019

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Dec 12, 2019
@cjroebuck
Copy link
Author

Thanks, using this version now, working well!

@danielyaa5
Copy link

I don't think this is working

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

No branches or pull requests

3 participants