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

Improve Logging #27

Closed
wants to merge 1 commit into from
Closed

Improve Logging #27

wants to merge 1 commit into from

Conversation

tts-sdrissen
Copy link
Contributor

Hi,

I would suggest, that only fetch the request data, if log is not off.
This way, if log is off, there will be no significant overhead.

Your initial plan with JSON-logs was good, because I could pick the data to compare.
But now, it is better to optional not log the timestamp. With this optional parameter, I can compare the log-file after running the test cases. If they are the same, the integration test is successful.

If you are approve this pull request, I would be very thankful, if you could release a new version, so I can use the new features.

Thanks for your time so far!

Viele Grüße, Simon

set default parameter of log to '' (nothing)
@Eun
Copy link
Owner

Eun commented Jul 15, 2023

I made some changes in #28, please take a look

@Eun Eun closed this Jul 15, 2023
@tts-sdrissen
Copy link
Contributor Author

I see you have been busy...
Unfortunately you created a race condition:
In server.js, line 59, you do logging after the request has ended, but it would be better to log after the data is fetched.
And why do you fetch the data, if log could be deactivated?

Your move makes the request to be logged after the request is fulfilled.
I saw, that a request can be fullfilled, and the next request can be send and fullfilled quicker than the logging of the previous request is marked as fulfilled... This leads to, that the lines in the log could be switched.

Like:

GET /request/2 ""
GET /request/1 ""

Expected:

GET /request/1 ""
GET /request/2 ""

@tts-sdrissen
Copy link
Contributor Author

Rewind... You do the logging before reply the result...
But nevertheless it could be, that the log is being written before the reply is send...
I would suggest to change server.js after line 52:
` server.on('request', (request, response) => {
if (config.log !== "") {
let data = '';

		request.on('data', (chunk) => {
			data += chunk;
		});
		
		let now = config.logTime ? `[${formatTime.format(new Date())}] ` : '';
		txtLogger.write(`${now}${request.method} ${request.url} ${JSON.stringify(data)}\n`);
	}

    request.on('end', () => {
        if (config.noCache) {

`

@Eun
Copy link
Owner

Eun commented Jul 17, 2023

The problem with your suggested change is that the log will be written before all data has been received.

@Eun
Copy link
Owner

Eun commented Jul 17, 2023

I suggest you fork the repo and run the test actions to see what I mean.

@tts-sdrissen
Copy link
Contributor Author

I see... Good point... Okay then... I think the problem is somewhat of an asynchronous handling with the log file writing.
Do you think it is a solution to flush to the log file, after writing a line?

https://nodejs.org/api/fs.html#filehandlesync

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