honeybadger-io / honeybadger-python

Send Python and Django errors to Honeybadger.
https://www.honeybadger.io/
MIT License
15 stars 25 forks source link

Return uuid from honeybadger.notify #140

Closed Kelvin4664 closed 1 year ago

Kelvin4664 commented 1 year ago

Closes #139

Kelvin4664 commented 1 year ago

One thing to note here is, We can only expect to see a return value when honeybadger.notify() is called with force_sync config turned on, since the api call is executed in separate threads rather than asynchronously when force_sync is off. To achieve a truly asynchronous function call, we'll need to resort to using async/await expression which is only available from python 3.5 or newer.

cc: @joshuap

subzero10 commented 1 year ago

One thing to note here is, We can only expect to see a return value when honeybadger.notify() is called with force_sync config turned on, since the api call is executed in separate threads rather than asynchronously when force_sync is off. To achieve a truly asynchronous function call, we'll need to resort to using async/await expression which is only available from python 3.5 or newer.

Isn't there a way to allow this with a callback method?

joshuap commented 1 year ago

One thing to note here is, We can only expect to see a return value when honeybadger.notify() is called with force_sync config turned on, since the api call is executed in separate threads rather than asynchronously when force_sync is off. To achieve a truly asynchronous function call, we'll need to resort to using async/await expression which is only available from python 3.5 or newer.

Isn't there a way to allow this with a callback method?

We could generate the UUID on the client-side in this case (so that it can return immediately), like we do for Ruby:

https://github.com/honeybadger-io/honeybadger-ruby/blob/1fa62bce0f0e35e0d2779c5a79f659356289f0d3/lib/honeybadger/notice.rb#L246

We'd need to make sure we use a random uuid generator like SecureRandom:

https://github.com/honeybadger-io/honeybadger-ruby/blob/1fa62bce0f0e35e0d2779c5a79f659356289f0d3/lib/honeybadger/notice.rb#L190

I am not sure what the best option is in Python—does it have a built-in method to generate a secure UUID?

Edit: I'm not completely sold on this, fwiw. There is an argument that the async approach would be better because you can confirm that the error was accepted by the API. The counter-argument is that you can display the UUID immediately without waiting for the error to be reported—my take is that most people will not switch to synchronous reporting just to use this feature in a server-side environment. Feel free to push back, I guess. 😁

subzero10 commented 1 year ago

We could generate the UUID on the client-side in this case (so that it can return immediately)

I didn't even know this was possible :)

I am not sure what the best option is in Python—does it have a built-in method to generate a secure UUID?

There's the uuid built-in package that can generate UUID v4.

Edit: I'm not completely sold on this, fwiw. There is an argument that the async approach would be better because you can confirm that the error was accepted by the API. The counter-argument is that you can display the UUID immediately without waiting for the error to be reported—my take is that most people will not switch to synchronous reporting just to use this feature in a server-side environment. Feel free to push back, I guess. 😁

If it's acceptable in the Python community, I'd go for the id returned using a callback method. It's a reliable solution and since it's a new feature, there are no backward compatibility concerns. Note: I wouldn't go for the current solution, which returns id only if force_sync is True.

joshuap commented 1 year ago

Note: I wouldn't go for the current solution, which returns id only if force_sync is True.

Agree

Kelvin4664 commented 1 year ago

One thing to note here is, We can only expect to see a return value when honeybadger.notify() is called with force_sync config turned on, since the api call is executed in separate threads rather than asynchronously when force_sync is off. To achieve a truly asynchronous function call, we'll need to resort to using async/await expression which is only available from python 3.5 or newer.

Isn't there a way to allow this with a callback method?

Can you explain with some code/pseudo code samples? Every approach i tried forces the function to act synchronously. I think this is a classical Python GIL Issue

Kelvin4664 commented 1 year ago

One thing to note here is, We can only expect to see a return value when honeybadger.notify() is called with force_sync config turned on, since the api call is executed in separate threads rather than asynchronously when force_sync is off. To achieve a truly asynchronous function call, we'll need to resort to using async/await expression which is only available from python 3.5 or newer.

Isn't there a way to allow this with a callback method?

We could generate the UUID on the client-side in this case (so that it can return immediately), like we do for Ruby:

https://github.com/honeybadger-io/honeybadger-ruby/blob/1fa62bce0f0e35e0d2779c5a79f659356289f0d3/lib/honeybadger/notice.rb#L246

We'd need to make sure we use a random uuid generator like SecureRandom:

https://github.com/honeybadger-io/honeybadger-ruby/blob/1fa62bce0f0e35e0d2779c5a79f659356289f0d3/lib/honeybadger/notice.rb#L190

I am not sure what the best option is in Python—does it have a built-in method to generate a secure UUID?

Edit: I'm not completely sold on this, fwiw. There is an argument that the async approach would be better because you can confirm that the error was accepted by the API. The counter-argument is that you can display the UUID immediately without waiting for the error to be reported—my take is that most people will not switch to synchronous reporting just to use this feature in a server-side environment. Feel free to push back, I guess. 😁

@joshuap We can pass a UUID generated on the client side to be retained on the server? I explored that option initially cos i see the ruby gem is doing it but couldn't find where token should be passed in the notice specification

And yes. Python has an inbuilt UUID module that can reliably generate uuid

joshuap commented 1 year ago

@Kelvin4664 yeah, it should work. I guess we need to update the notice spec/schema. In any case, just do the same thing as the Ruby gem and it should work.

Kelvin4664 commented 1 year ago

@subzero10 @joshuap can you take another look? I updated to generate uuid on the client side, added and updated a few test cases.

subzero10 commented 1 year ago

LGTM!