[FEAT]: should 304 conditional requests be logged as errors?

[jeffwilcox]

Describe the need

Not sure if this is a request to "change the default behavior" or "allow some magical flags to customize"...

We make use of conditional requests a lot. We've had to patch the standard Octokit opinionated plugin choices with a version where we treat a 304 error.status as informational instead of an error - our server logs were full of all the conditional requests that "successfully" returned 304s.

Not sure if this is something that's come up before, but would love to know how common it is that others do not want to see endless log spew such as this...

GET /repos/contosoenterprise/my-test-repo - 304 with id F178:35CFAD:10ED6DE5:100C9A83:6722BC7D in 243ms

Just a comment.

SDK Version

N/A

API Version

N/A

Relevant log output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[wolfy1339]

there is no error level logging in this module, it is either log level debug or info

[jeffwilcox]

I am specifically looking at this line:

https://github.com/octokit/plugin-request-log.js/blob/268c62891d89b2d4b094bf6c7d179b88baa84842/src/index.ts#L30

octokit.log.error(
          `${requestOptions.method} ${path} - ${error.status} with id ${requestId} in ${
            Date.now() - start
          }ms`,
        );

This is what we've overridden in our own Octokit scripts to log this as info and not error if error.status === 304.

[wolfy1339]

The root of this issue is not in this module but rather in @octokit/request

request.js/src/fetch-wrapper.ts

Lines 124 to 131 in 1ffa99b

	if (status === 304) {
	octokitResponse.data = await getResponseData(fetchResponse);
	throw new RequestError("Not modified", status, {
	response: octokitResponse,
	request: requestOptions,
	});
	}

We throw an error specifically if the status is a 304

All this plugin does is catch errors from requests and logs them with the error level

[jeffwilcox]

OK, fair. We'll just keep patching in our world. Thanks!

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀