Skip to content

Enable JSON logging#65

Merged
trusche merged 20 commits intotrusche:masterfrom
coverwallet:feature/json-logging
Dec 9, 2018
Merged

Enable JSON logging#65
trusche merged 20 commits intotrusche:masterfrom
coverwallet:feature/json-logging

Conversation

@bustikiller
Copy link
Copy Markdown
Collaborator

@bustikiller bustikiller commented Nov 2, 2018

This pull request adds the option to log HTTP requests as JSON.

NOTE: http4 does not support ruby < 2.3.X, so the build for that combination has been flagged as "allow_failure"

@trusche
Copy link
Copy Markdown
Owner

trusche commented Nov 5, 2018

Hi @bustikiller, I'd be happy to accept this with existing specs passing and new specs added to cover the added behaviour. Currently it breaks compact logging. See for example https://travis-ci.org/trusche/httplog/jobs/450821360.

@bustikiller
Copy link
Copy Markdown
Collaborator Author

Hi @trusche, this is still work in progress, I will fix the existing tests and write new tests for the added feature. Thanks!

@trusche
Copy link
Copy Markdown
Owner

trusche commented Nov 5, 2018

Cool, just assign to me when you consider it done.

@bustikiller
Copy link
Copy Markdown
Collaborator Author

@trusche I think I have covered all the changes I wanted to do, along with the test cases. I cannot assign the PR to you because I don't have permissions.

I would love to hear some feedback from you.

@trusche trusche assigned trusche and unassigned bustikiller Nov 7, 2018
@bustikiller
Copy link
Copy Markdown
Collaborator Author

@trusche did you have a chance to review this PR? I added a small change to identify application/pdf as binary, as it was causing some troubles with the transformation to JSON. It may be aligned with #61.

@trusche
Copy link
Copy Markdown
Owner

trusche commented Nov 26, 2018

Hi @bustikiller, sorry I've been swamped. I'll try to get around to this this weekend.

Copy link
Copy Markdown
Owner

@trusche trusche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of repeated code across the various adapters now, all just calling various HttpLog methods in much the same order. It might be a good time to refactor that so that each adapter only needs to call a unified method and let the HttpLog module sort out how to log. The only thing that would have to be called separately from each adapter would be the connection logging.

I'm going to take a stab at that unless you want to.

Comment thread .travis.yml
Comment thread lib/httplog/version.rb
Comment thread README.md
@trusche trusche merged commit 8c559e1 into trusche:master Dec 9, 2018
@trusche
Copy link
Copy Markdown
Owner

trusche commented Dec 9, 2018

Hi @bustikiller I've merged this into master and then added some refactorings on top. Would you let me know if master is working ok for you? I'm aiming to release this as 0.1.20 in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants