-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add logging and recording of requests #170
base: main
Are you sure you want to change the base?
Conversation
@joanlopez I would like your thoughts on this please! |
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
upvote for this feature, really missing this essential feature! @joanlopez |
@aperezg @joanlopez is there anything else I need to do before this can get reviewed? |
It's fine for now, @tateexon - just trying to book some time to review it carefully! Thanks! 🙇🏻 |
Sounds good. I will continue with my fork for now to unblock my QA team. |
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
- Handle cases where the dumpCh is full - Use bytes.NewReader
Base64 encode the body and attach it to the log that is output if there is a body
I think I have hit all the review comments so far. Anything else @asad-urrahman ? Thanks for the reviews so far! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All Good
when is this getting merged in? |
internal/app/cmd/cmd.go
Outdated
@@ -135,6 +141,9 @@ func runServer(cfg killgrave.Config) server.Server { | |||
proxyServer, | |||
cfg.Secure, | |||
imposterFs, | |||
server.PrepareAccessControl(cfg.CORS), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I don't think this refactor is related (and/or strictly required) with the purpose of this pull request. Plus, I'm not even sure we really want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed so I can propagate cors here: https://github.com/friendsofgo/killgrave/pull/170/files#diff-09b422118d0773bb206a9e4cf4cbbaeeb017894e4007525bfbeb8d0163b3c824R102-R106
If we don't propagate it later then we can't get cors and the logging at the same time.
internal/server/http/dump.go
Outdated
} | ||
} | ||
|
||
// Copied from the gorilla/handlers package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all those contents copied over from gorilla/handlers
, I'd love to see them:
- Isolated as much as possible (ideally in a different package, or at least in a different file/s).
- Accompanied with the reference they were copied from; like the Git hash, so we can track future changes easily.
- Accompanied with the corresponding license, as I think
gorilla/handlers
is licensed with BSD-3 and it requires to retain copyright. - Unit tests, if any available in the original codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even better, have you considered re-using LoggingHandler
instead? 🤔 What prevents you from doing so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LoggingHandler on its own does not provide the body. We are copying over the parts we need in order to build basically the Logginghandler plus the body when wanted. If it provided a way to print the body then we would not need to copy any of the gorilla handler logging code but sadly it does not. Had they exposed more of their functions then we would not need to copy them as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the licensing mismatch how would you like this handled? I can copy over their license into the license for this project but I have never dealt with this specifically before so not sure how to best handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will get the code moved to its own gorilla/handlers directory along with the tests copied in as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I dropped all the gorilla code and just went with a json logger for now. We can add other formats later when/if wanted. This keeps us from having to deal with multiple license fun and we don't have copy pasted code from another repo. We just reuse the gorilla CustomLoggingHandler to log/dump requests when needed.
internal/server/http/dump.go
Outdated
} | ||
|
||
// copied from gorilla/handlers and modified to add the body | ||
// writeLog writes a log entry for req to w in Apache Common Log Format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure we want to use Apache Common Log Format, or perhaps something simpler. Or eventually something plugable/selectable, leaving the choice to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not want to use the apache common log format we can get rid of a lot of this and build our own custom handler. Was just doing what Asad wanted in his reviews. I am not tied to the format nor do I care as long as it is readable. If you have a preference for a default format please let me know and I will go with that so long as it will support the ability for a body to be present when wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ripped out for now.
internal/server/http/dump.go
Outdated
r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) // Reset the body | ||
body := string(bodyBytes) | ||
// if content is binary, encode it to base64 | ||
if isBinaryContent(r) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you already have the body in bytes, I'd suggest to follow the opposite approach, only use something else other than Base64 if possible (like valid JSON contents), but defaulting to Base64, which is safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reversed in: 6459695
internal/server/http/dump.go
Outdated
if s.logLevel == 0 && !shouldRecordRequest(s) { | ||
return "" | ||
} | ||
bodyBytes, err := io.ReadAll(r.Body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see some limits here. Does it really make sense to write the body over and over, especially if large? 🤔 What's the purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense when you want the body logged out for tests to grab and use. This is provided in other mocking solutions in some way.
This is kind of the whole reason for the PR, I need the body for tests and yes it could be large and yes I will still want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely put a limit if needed. It would need to be configurable for when I want a very large body. What would you say a good limit would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a io.LimitedReader to read up to a configurable maximum bytes now: 78a8ae0
internal/server/http/dump.go
Outdated
// Handle the case where the channel is full | ||
log.Println("Channel is full, dropping request and logging it instead:") | ||
return fmt.Errorf("request dump channel is full") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to do so?
Couldn't you just record the request asynchronously?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is getting recorded asynchronously but if that asynchronous channel is full we need to handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned this up so it just logs an error if the channel is full
internal/server/http/dump.go
Outdated
} | ||
|
||
// Goroutine function to write requests to a JSON file | ||
func RequestWriter(filePath string, requestChan <-chan *RequestData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this function lacks proper control of its lifecycle, graceful shutdown, etc. Same for the channel, etc; please review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will look into adding a context to pass in and potentially move the for loop and file open outside of this function since that is the only way I think I can properly add that control but the server will need to pass the context along. I think I can do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked this to have a context and a wait group to handle graceful shutdown.
internal/server/http/dump.go
Outdated
func RequestWriter(filePath string, requestChan <-chan *RequestData) { | ||
file, err := os.OpenFile(filePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | ||
if err != nil { | ||
log.Fatalf("Failed to open file: %+v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see handling of this to happen before, in an outer surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been moved. We now use an io.Writer instead so the file open and defer are handled at higher layers.
internal/server/http/dump.go
Outdated
} | ||
|
||
// GetRecordedRequests reads the requests from the file and returns them as a slice of RequestData | ||
func getRecordedRequests(filePath string) ([]RequestData, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you adequately abstract the destination of the logs through io.Writer
, you won't need to use real files for testing, which isn't.. ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you are saying makes sense, this function should have been in the test file anyways and not here since it is only being used in the tests.
internal/server/http/server.go
Outdated
} | ||
|
||
// NewServer initialize the mock server | ||
func NewServer(r *mux.Router, httpServer *http.Server, proxyServer *Proxy, secure bool, fs ImposterFs) Server { | ||
func NewServer(r *mux.Router, httpServer *http.Server, proxyServer *Proxy, secure bool, fs ImposterFs, CORSOptions []handlers.CORSOption, logLevel int, dumpRequestsPath string) Server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to keep adding more and more options (three this time), I'd love to see options as functional arguments, in a separate package. Especially for those that aren't required, the rest can be kept as regular args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a separate serverconfig package to handle extra args.
internal/server/http/server_test.go
Outdated
@@ -67,7 +70,7 @@ func TestBuildProxyMode(t *testing.T) { | |||
imposterFs, err := NewImposterFS("test/testdata/imposters") | |||
require.NoError(t, err) | |||
|
|||
server := NewServer(router, httpServer, proxyServer, false, imposterFs) | |||
server := NewServer(router, httpServer, proxyServer, false, imposterFs, nil, 0, "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smell that points to https://github.com/friendsofgo/killgrave/pull/170/files#r1686772983
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now handled with a serverconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tateexon,
I finally managed to do an initial review, and the TL;DR of my feedback is that this pull request is not only too large, but also mixes things that aren't strictly needed together/dependent among them.
So, before addressing the detailed comments, I'd suggest you to split this into multiple, independent PRs, including the smaller refactors, other improvements (like the uncaught error), and even the two flags added here (separate each, please).
Beyond making it much easier to review, and in the future to track changes, I think that some of the minor changes could be merged straight away (as-is), and we would really appreciate such contributions, while in parallel we can discuss the details around logging, and consider if we really want to add support for dumping requests at all (which I have my doubts, to be honest).
Meanwhile, I'll ping @aperezg, cause while I'd be happy and comfortable with merging your smaller changes, I'd love to get their approval as well for the larger changes.
Thanks! 🙇🏻
TBH, I'd love to see this merged sooner rather than later. However, the set of changes here is very large, and some are trickier to review, discuss and approve than others. That's why I just suggested @tateexon to split this into smaller PRs. From your side @bigmeech, is there anything you're more interested about? Logging? Dumping? The minor fixes? Thanks! |
007a1c4
to
7c5b2d5
Compare
9471649
to
dbd4701
Compare
Use server config package in cmd, NewServer, and tests Move gorilla handlers logging copy paste to its own package Add gorilla license to the license file
Convert channel to *[]byte to make handling future log formats easier
add configuration for this
@joanlopez Thank you very much for the review! I broke out a PR for the makefile and dockerfile changes. I think I have hit most of your review comments and that brought a lot of change. I would like another review before I go in and add more tests just in case it needs a lot more change. Some of the things that have changed:
|
Hi @joanlopez Thanks for the work you have done in the PR. To answer your question actually there are a few stuffs I'd be interested in but I think the most important for me personally is responding to the client with a 400 bad request when the schema validation fails instead of only reporting the failed validation on the server logs. (at least this is what happens in the version i am using). Not sure how big of a change that would be. Thanks. |
Added two new command line arguments:
-l
for logging level, defaults to 0, 1 adds requests, 2 adds the request body-d
to dump requests into a the specified fileThis is needed for cases when a test needs access to what is posted as a request to the mock but we don't know the exact data, like a hash that was posted but need that hash later on in the test to post to another service. We don't need the responses dumped because the test should have access to the imposters they are using and thus already have access to that data.
I think this also covers what was originally wanted in #100 but maybe not exactly how it was imagined there.
I added a few changes to the Makefile and Dockerfile to make local testing a bit easier.
I made the request writer use a channel and run in a go function so it wouldn't run into issues of multiple concurrent requests trying to write into the dump file at once.
I fully realize this could cause the dump file to get very very large if this is used incorrectly in something like a load test but I am not sure how to handle that better outside of doing some kind of rotating log. I would hope people wouldn't use it for a case like that but I don't know what all the kinds of users of the tool are. I welcome all suggestions.