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

Add logging and recording of requests #170

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

tateexon
Copy link

@tateexon tateexon commented Jun 14, 2024

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 file

This 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.

@tateexon tateexon changed the title Add logging and reording of requests Add logging and recording of requests Jun 14, 2024
@tateexon
Copy link
Author

@joanlopez I would like your thoughts on this please!

tateexon added a commit to smartcontractkit/chainlink-testing-framework that referenced this pull request Jun 14, 2024
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
@iljapavlovs
Copy link

upvote for this feature, really missing this essential feature! @joanlopez

@tateexon
Copy link
Author

@aperezg @joanlopez is there anything else I need to do before this can get reviewed?

@joanlopez
Copy link
Member

@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! 🙇🏻

@tateexon
Copy link
Author

Sounds good. I will continue with my fork for now to unblock my QA team.

tateexon added a commit to smartcontractkit/chainlink-testing-framework that referenced this pull request Jun 20, 2024
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
tateexon added a commit to smartcontractkit/chainlink-testing-framework that referenced this pull request Jun 20, 2024
Note: This requires approval of: friendsofgo/killgrave#170 which may still need changes.
internal/app/cmd/cmd.go Outdated Show resolved Hide resolved
internal/config.go Outdated Show resolved Hide resolved
internal/server/http/dump.go Outdated Show resolved Hide resolved
internal/server/http/dump.go Outdated Show resolved Hide resolved
internal/server/http/server.go Outdated Show resolved Hide resolved
internal/server/http/dump.go Outdated Show resolved Hide resolved
internal/server/http/dump.go Outdated Show resolved Hide resolved
internal/server/http/dump.go Show resolved Hide resolved
internal/server/http/dump.go Outdated Show resolved Hide resolved
@tateexon
Copy link
Author

I think I have hit all the review comments so far. Anything else @asad-urrahman ? Thanks for the reviews so far!

Copy link

@asad-urrahman asad-urrahman left a comment

Choose a reason for hiding this comment

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

All Good

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.

None yet

4 participants