alexrudall / ruby-openai

OpenAI API + Ruby! πŸ€–β€οΈ NEW: Assistant Vector Stores
MIT License
2.73k stars 321 forks source link

Add log_errors option to enable/disable logging of errors. #406

Closed lalunamel closed 5 months ago

lalunamel commented 9 months ago

All Submissions:

Hello - first contribution here! πŸ‘‹

This PR adds the log_errors option, which enables/disables the MiddlewareErrors middleware, controlling whether or not errors are logged when they are encountered

The PR includes:

Motivation: I'd like to use ruby-openai because it looks like a wonderful, well-maintained project. Unfortunately, the library logs errors when they are encountered. The information logged includes the response body sent over from OpenAI, which might conceivably include part of the prompt or response. In order to avoid leaking that information to the system orchestrating the request, I'd like to disable logging of errors.

Testing: I've run all the rspec tests locally and tested the log_errors functionality against my own local project to confirm it works and does not impact behavior anywhere else.

Please let me know if there are any changes you'd like to see in the PR before it can be accepted!

alexrudall commented 9 months ago

Thanks for this! ~I'd like to make it a bit more powerful - what about instead adding a error_response: param, making that default to the current logic (ie everything in the rescue in MiddlewareErrors), and then people can pass false to that (your usecase), OR their own custom logic as a proc with how to handle Faraday::Errors. So they could make their own logic for logging errors easily.~

EDIT: Thought about this some more. Actually I prefer your implementation. And I wonder if it would be safer to turn this off by default? Tradeoff between making it as easy as possible for junior devs to understand what is happening, and leaking data to logs by default πŸ€”

lalunamel commented 9 months ago

Thanks for your quick reply!

I agree with your sentiment about not logging errors by default. Didn't want to be too disruptive in my first PR here, but if you approve, then πŸ‘ . Added a commit to change the behavior.

If you are πŸ‘ on this PR, feel free to approve and merge.

lalunamel commented 9 months ago

Hi @alexrudall - any thoughts on this?

lalunamel commented 8 months ago

Ping @alexrudall - let me know if there's anything else to do here!

lalunamel commented 7 months ago

Ping @alexrudall

alexrudall commented 6 months ago

Thanks so much for this @lalunamel and apologies for the delay in response. I consider this a breaking change and will merge it soon in next major release.

lalunamel commented 6 months ago

Woo!