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

Logging mechanism for debugging #126

Closed
reden-tm opened this issue Dec 17, 2014 · 11 comments
Closed

Logging mechanism for debugging #126

reden-tm opened this issue Dec 17, 2014 · 11 comments
Labels

Comments

@reden-tm
Copy link

Ruby has a logger and Python has a logger.

We're using the recurly-client-php library, and we really could use a debug mode logger or a general logger to trace logic errors with our usage of the recurly client.

Are there any plans for this?

@drewish
Copy link

drewish commented Dec 17, 2014

No plans for it but we'd be be totally open to PRs.

@reden-tm
Copy link
Author

Cool.

Mind if I refactor Recurly_Client-> _sendRequest() to use curl_setopt_array() to allow verbose curl?

@drewish
Copy link

drewish commented Dec 17, 2014

Just for to clean up all those curl_setopt calls? Yeah that seems like a reasonable change. Were you thinking of controlling verbose via a property on Recurly_Client?

@reden-tm
Copy link
Author

Yeah, I was thinking about controlling verbosity via a property on the client. I think I'll split this into two PR's. One with a simple property check:

if($this->curlopt_verbose) curl_setopt($ch, CURLOPT_VERBOSE, true);

And maybe another for the cleanup, because I probably won't get to the cleanup today.

@drewish
Copy link

drewish commented Dec 17, 2014

What do you think about just calling the property verbose? i think curl just prints to standard err right? Wondering if it makes sense to expose CURLOPT_STDERR... I guess that could be a separate PR.

@reden-tm
Copy link
Author

Yeah, verbose is better, but now I think the option is not really that useful. I see why there's a separate branch for verbose cURL; it's much easier to apply the patch to my local dev and use that.

I really like the logging available in the Ruby and Python clients, but it looks like that would be a fairly large PR.

For example, the Ruby code checks to see if a logger exists and decides what information to log.

          if Recurly.logger
            Recurly.log :info, "===> %s %s" % [request.method, uri]
            headers = request.to_hash
            headers['authorization'] &&= ['Basic [FILTERED]']
            Recurly.log :debug, headers.inspect
            if request.body && !request.body.empty?
              Recurly.log :debug, XML.filter(request.body)
            end
            start_time = Time.now
          end

If I add logging into the PHP client, do you think I should mimic the pattern and output from the Ruby client?

@drewish
Copy link

drewish commented Dec 17, 2014

I'm wondering if it's worth finding a component to avoid re-inventing the wheel. maybe symphony has a component or something? Seems like there's probably something popular that we could add as a dev dependency and then let the caller inject it into the client... but yeah kind of a PITA to do it right.

@austinheap
Copy link

@drewish +1 to leverage Monolog

@reden-tm
Copy link
Author

reden-tm commented Feb 6, 2015

@drewish and @austinheap , I'm working on a patch for PSR-3 logging support now. You should see it soon.

@drewish
Copy link

drewish commented Feb 6, 2015

@reden-tm awesome! i'm happy to look at any in progress commits.

reden-tm added a commit to Theatermania-Inc/recurly-client-php that referenced this issue Feb 9, 2015
@aaron-junot
Copy link

Hello all. I would like to point out that we recently disabled logging in production for the Python and Ruby client libraries as it is a security concern.

One of the benefits of using Recurly as a platform is that you lessen your compliance scope (PCI, etc.) by letting us handle the security of processing and storing your customers' PII and credit card data. By logging the information sent to and from the Recurly servers, you now open yourself up to various security and compliance risks, hence we do not advocate logging production data from our libraries.

For that reason, I'm going to go ahead and close out this issue and the related PR (without merging it). Please be aware of the risks of using forks of this library that log production data, and please take all precautions to protect your customers' data.

@bhelx bhelx added the V2 V2 Client label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants