new10com / axios-logger

Library that should be used as axios request/response interceptor for logging request/response details
MIT License
14 stars 2 forks source link

silence body logging for a single request. #31

Closed xenoterracide closed 2 years ago

xenoterracide commented 2 years ago

Not sure if this is viable, but I have a request that returns a massive payload. instead of logging it, I would prefer to silence it. We use a singleton(ish) axios instance, because... well, whatever. I could overwrite the interceptors temporarily, maybe there's an option that can be handled by the interceptor. Probably an opportunity for an option to be passed to the logger.

xenoterracide commented 2 years ago

this apparently is happening with logging the body in an error...

xenoterracide commented 2 years ago

anyways, it's a 19M body, but I don't actually want to squelch the body logger for everything. Just things over a certain content size, or probably unknown size.

xenoterracide commented 2 years ago

The logging slowed it down so much though the request actually failed. (sorry for so many messages, work firewall doesn't allow comment editing)

azakordonets commented 2 years ago

@xenoterracide would silensing body configuration work for you ?

// If you want to ommit request headers and body
const config = { response: { shouldLogBody: false} }

?

xenoterracide commented 2 years ago

yes, but I want it only on a specific request, but I'd rather not recreate the interceptors or the instance of logger for this, so if there was something I could set on the request itself... which is kind of why I'm suggesting a size limit

{ response: { maxLogBodySize: 1024 }}
xenoterracide commented 2 years ago

note: also have a problem where there's a layer between us and axios, which I think is stripping configs. so setting in the interceptors themselves is difficult because I would have to make a match url regex...

xenoterracide commented 2 years ago

maxLogContentLength is probably a better name

azakordonets commented 2 years ago

@xenoterracide give it a try in 0.2.9 version )

xenoterracide commented 2 years ago
❯ npm explain @new10com/axios-logger
@new10com/axios-logger@0.2.9

doesn't actually seem to work :'(

Content-Length | 314290

still has the body logged

const axiosLogger = AxiosLogger.default({
  obfuscation: {
    obfuscate: true,
  },
  response: {
    maxLogContentLength: 1024,
  },
})
xenoterracide commented 2 years ago
function withTracingInterceptor(axiosInstance: AxiosInstance): AxiosInstance {
  axiosInstance.interceptors.request.use(
    config => axiosLogger.logRequest(config),
    error => axiosLogger.logErrorDetails(error),
  )
  axiosInstance.interceptors.response.use(
    response => axiosLogger.logResponse(response),
    error => axiosLogger.logErrorDetails(error),
  )
  return axiosInstance
}
xenoterracide commented 2 years ago

by the way, I notice that the comment says in kb, shouldn't it just be in bytes? since that's how content length itself is measured? https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Length

azakordonets commented 2 years ago

@xenoterracide you are right about the message - i missed that part to fix it. I have updated it in 0.2.10 version. The way I was calculating the length of the body is following :

const bodyLengthInBytes = Buffer.byteLength(bodyAsString)
      if (bodyLengthInBytes > maxLogContentLength) {
        return `Body is too long to be displayed. Length: ${bodyLengthInBytes} bytes. Max length: ${maxLogContentLength} bytes.`
      }

The thing is that I'm not checking Content-Lenght header because parser of the body does not take headers as an input. I might be wrong on this side, but I thought that calculating size this way will be accurate. Are there cases when this would not work ?

I used a very simple unit test to verify that this work:

it(`should return warning message when body length is longer then set in config`, () => {
      const body = 'Hello how are you?'.repeat(100)
      expect(
        formatter.prettyFormatBody({ body, maxLogContentLength: 10 })
      ).toEqual(
        `Body is too long to be displayed. Length: 1802 bytes. Max length: 10 bytes.`
      )
    })
xenoterracide commented 2 years ago

No, that should work. I think... That doesn't read all of them into memory does it?... Like make a second copy. I'm on my phone right now so I can't check.

I think if that's how it's going to be measured then the API should be a little bit different in order to make it clear that we're dealing in kilobytes or whatever... I just found it a little unintuitive and if I hadn't read the code I wouldn't have noticed that it wasn't in bytes directly.

azakordonets commented 2 years ago

I have fixed the message about kilobytes and now it's all in bytes. About performance of this solution - to be honest, I'm not sure how Buffer works under the hood, but I will try to run some performance benchmarks later on. For now we can consider this as a quick fix just to unblock you. Later on I will try to improve this.

xenoterracide commented 2 years ago

probably the most performant is to introspect the response content-length header first, and then fallback to that if necessary.

e1-api-1  |   Method: @GET
e1-api-1  |   Status: 200  OK
e1-api-1  |   Headers
e1-api-1  |   ┌ server: "mithril"
e1-api-1  |   ├ date: "Thu, 02 Jun 2022 15:23:38 GMT"
e1-api-1  |   ├ content-type: "application/json"
e1-api-1  |   ├ content-length: "19247463"
e1-api-1  |   ├ connection: "close"
e1-api-1  |   ├ cache-control: "max-age=3600"
xenoterracide commented 2 years ago

it would appear that memory usage has gone up, not sure to what exactly but this code runs in 128M of ram without this. Also I think it conflicts in a weird way with shouldLogBody: true it seems like no body's were being logged when I had that set.

azakordonets commented 2 years ago

Will need to look into optimization of it based on header indeed

xenoterracide commented 2 years ago

Yeah, as it is, doesn't help me sadly. Oh well, I conditionalized the should log for now

xenoterracide commented 2 years ago

so, any chance for having this based on header if present soon?

xenoterracide commented 2 years ago

also I did this, and the big huge response is still logged, but I find that unintuitive

export const axiosLogger = AxiosLogger.default({
  obfuscation: {
    obfuscate: true,
  },
  response: {
    shouldLogBody: true, // actually set by env
    maxLogContentLength: 32767,
  },
})
azakordonets commented 2 years ago

@xenoterracide I have refactored how body length calculation happens ( now it's based on Content-Length header value ) . It's published in 0.2.12 version. Could you give it a try and see how it works ? Keep in mind that maxLogContentLength is present in both request and response configurations. So, if you want to "cut" both request and response - it has to be set in both configs.

xenoterracide commented 2 years ago

tested, looks good to me. thanks!

xenoterracide commented 2 years ago

oh, this is not at all important, but if you ever wanted to print the values in human readable, like 19M instead of 19247467 that'd be cool. Not important or blocking though.