search

quarkiverse / quarkus-github-app

Develop your GitHub Apps in Java with Quarkus.
https://docs.quarkiverse.io/quarkus-github-app/dev/index.html
Apache License 2.0
60 stars 27 forks source link

Better handling of GitHub's API outage #576

Closed The-Huginn closed 3 months ago

The-Huginn commented 4 months ago

Hi,

recently an GitHub API outage happened. It would be nice to log this fact clearer, thus a user doesn't have to search for the problem. Currently, when my bean is invoked, requiring a configfile as parameter of the method, during the fetching the error occurred. The exception would have to be caught here. But it's unchecked HttpException, thus a general IOException would need to be caught and compared.

I have created an issue in underlying GitHub API library, please see to make it easier to detect.

Please, let me know, what do you think about it.

bitwiseman commented 3 months ago

https://github.com/hub4j/github-api/pull/1813 closes the GitHub API issue and has been released in v1.320.

gsmet commented 3 months ago

@The-Huginn how would you expect it to behave on the Quarkus GitHub App side?

The-Huginn commented 3 months ago

I am currently just catching the exception and logging the info. However, I am not sure yet, how to approach propagating it to multiplexer. Which would then not even call the original method. As currently, it would only log the information and call the method with null.

This is obviously just case for fetching file. As the user can manually call GitHubConfigFileProvider and he would not receive this info.

Please, let me know, what would be the best approach here @gsmet

gsmet commented 3 months ago

Where are you catching the exception? Personally when having this exception I would probably want to throw and cancel the whole call.

Then we have DefaultErrorHandler (that you can customize) where we could maybe print a customized message?

But I'm not entirely sure how you want to handle this case.

Another option could be to record the events and retry them later but sometimes order is important so I'm not entirely sure how this would go.

The-Huginn commented 3 months ago

I have gone over the exact stacktrace, that I provided in the original issue so this is the commit so far.

I will try to see what DefaultErrorHandler does. The thing what happened is, that the event from GitHub came, however consequent call back to github API (to retrieve file from @ConfigFile) failed. So you would like to catch the exception in the multiplexer and then Log this fact? I am not sure, if throwing exception is necessary.

gsmet commented 3 months ago

Maybe have a look at what I did here: https://github.com/quarkiverse/quarkus-github-app/pull/594

I avoided the wrapping in the config download and then I think everything should be handled properly. You will still get your error message but at least, you don't have the HTML in your stacktrace and you have an indication that the APIs are not available.

Does it make sense? If you have a better idea for the wording of the message I added, I'm all ears :).

The-Huginn commented 3 months ago

Yes, absolutely. This way, even when retrieving the file manually, you would be able to catch the exception. Great work, thanks for showing it to me!

gsmet commented 3 months ago

@The-Huginn I released 2.5.1 with this + forcing HTTP 1.1.

The-Huginn commented 2 months ago

Thanks for that, will update our tooling