Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Logger/tracer middleware: store responseBody once and cap its size #1697

Closed
fkaleo opened this issue Feb 28, 2020 · 1 comment
Closed

Logger/tracer middleware: store responseBody once and cap its size #1697

fkaleo opened this issue Feb 28, 2020 · 1 comment
Assignees
Labels

Comments

@fkaleo
Copy link
Contributor

fkaleo commented Feb 28, 2020

I think we should make a few more improvements here, though they are slightly out of scope.

  1. both the logging and tracing middleware are capturing the responseBody if the request status >= 400. we dont need two copies of the body.
  2. I really think we should set a cap on the number of bytes to capture. if the something goes wrong and a really large response is written we are going to cause problems for our logging and tracing platforms if we try and write 100's of KB or larger.

to address these i suggest

  1. merge the tracing and logging middleware into one, or use a shared responseWriter wrapper.
    for a shared responseWriter you can just check if current responseWriter is already a 'CaptureErrorResponseWriter' or not to see if you need to updated it. eg
  var wrappedRW *CaptureErrorResponseWriter
  if wrappedRW, ok := ctx.Resp.(*CaptureErrorResponseWriter); !ok {
      wrappedRW = &CaptureErrorResponseWriter{ResponseWriter: ctx.Resp
  }
  ....
  errBody := wrappedRW.ErrorBody()
  1. only put up to 1k of data into the captured errBody, and then additionally if we need to decompress it, only return the first 1k of data after decompression. this will work fine with gzip. It will decompress what it can then return an error. So if you get decoded data back, just ignore the error.

Originally posted by @woodsaj in #1693 (comment)

@fkaleo fkaleo self-assigned this Feb 28, 2020
@fkaleo fkaleo modified the milestones: sprint-8, sprint-9 Feb 28, 2020
@fkaleo fkaleo removed this from the sprint-9 milestone Mar 16, 2020
@stale
Copy link

stale bot commented Jun 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 14, 2020
@stale stale bot closed this as completed Jun 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

1 participant