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

Truncated response body using Sidekiq dashboard w/New Relic #4440

Closed
pelargir opened this issue Jan 29, 2020 · 14 comments
Closed

Truncated response body using Sidekiq dashboard w/New Relic #4440

pelargir opened this issue Jan 29, 2020 · 14 comments

Comments

@pelargir
Copy link

@pelargir pelargir commented Jan 29, 2020

Ruby version: 2.6.5p114
Sidekiq / Pro / Enterprise version(s): 6.0.4

We're using newrelic_rpm 6.8.0.360 to add JavaScript instrumentation to our Rails 5.2.4.1 app. After upgrading to Rack 2.1.2 the dashboard went blank. Viewing the page source, the HTML stops in the middle of the New Relic JS code near the top of the page (see screenshot).

Apparently, Rack::Response no longer overwrites the Content-Length header, which means an incorrectly configured Content-Length header is now exposed. See rack/rack#1520 (comment)

Is it possible Sidekiq is setting Content-Length prior to New Relic adding its instrumentation, resulting in an incomplete page load since the New Relic code takes up the entire Content-Length before the remaining body can be sent?

As soon as I remove New Relic from our app the dashboard starts working again.

view-source_https___www_candlescience_com_admin_sidekiq

@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Jan 29, 2020

Should NewRelic be instrumenting Sidekiq's pages at all? Maybe it should exclude /sidekiq/*.

See

resp[1][CONTENT_LENGTH] = resp[2].sum(&:bytesize).to_s

Added here with no explanation:

9ae228c

I just tested removing those two lines (315 and 317) and the Sidekiq UI appears to work without them in both Rack 2.0.8 and 2.1.2. Unfortunately I have no idea whether this is a good idea or not.

@pelargir

This comment has been minimized.

Copy link
Author

@pelargir pelargir commented Jan 29, 2020

Yes, New Relic Browser instrumentation should probably be excluded from Sidekiq::Web by default. They give examples of how to do this in a Rails controller and a Sinatra app, but not a pure Rack app. I'm thinking a middleware might work.

@pelargir

This comment has been minimized.

Copy link
Author

@pelargir pelargir commented Jan 29, 2020

Still trying to turn off New Relic for Sidekiq::Web using this:
https://docs.newrelic.com/docs/agents/ruby-agent/api-guides/ignoring-specific-transactions#page-load-timing-rum

In the meantime, to get running I've created a monkey patch that deletes the Content-Type header from responses coming from Sidekiq::Web. Hope this helps someone:

require 'sidekiq/web'

module RemoveContentLengthHeader
  def call(env)
    resp = super
    resp[1].delete('Content-Length')
    resp
  end
end

Sidekiq::WebApplication.prepend(RemoveContentLengthHeader)
@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Jan 29, 2020

If it works without NewRelic, please open an issue with them too. Maybe Sidekiq::Web shouldn't be calculating Content-Length but I'd like someone to explain why I should change code that has worked fine for years now.

@pelargir

This comment has been minimized.

Copy link
Author

@pelargir pelargir commented Jan 29, 2020

I'll do that, however I suspect the only reason it's worked fine until now was due to older versions of Rack overwriting the Content-Length. See rack/rack#1520 (comment)

This did not happen with Rack versions prior to 2.1.2 (even with New Relic instrumentation installed).

@mperham mperham closed this in 675e40c Jan 30, 2020
@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Jan 30, 2020

You've convinced me. No sense in calculating something unnecessarily when we can let Rack do it correctly for us.

@ArturT

This comment has been minimized.

Copy link

@ArturT ArturT commented Jan 31, 2020

@mperham When do you plan to release sidekiq 6.0.5?

@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Jan 31, 2020

I believe the workaround today is gem 'rack', '~> 2.0.8'. Because there is a workaround, I don't feel the need to ship a quick fix. Please correct me if there is a more urgent need for some reason.

@ArturT

This comment has been minimized.

Copy link

@ArturT ArturT commented Jan 31, 2020

@mperham I assumed you will be releasing a new sidekiq soon because this PR was merged. That's fine if you have a bit longer release timeline. gem 'rack', '~> 2.0.8' works fine. Thanks.

@flanger001

This comment has been minimized.

Copy link

@flanger001 flanger001 commented Feb 4, 2020

This helped us a lot too, thank you. @mperham any chance you can backport this into a 5.x version? I can't upgrade to 6 yet. Locking rack is fine, but I thought I'd ask.

mperham added a commit that referenced this issue Feb 4, 2020
@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Feb 4, 2020

I've committed a gemspec update to the 5-x branch which you can run. It has two commits in the last year so it should be very stable.

gem 'sidekiq', branch: '5-x'
@flanger001

This comment has been minimized.

Copy link

@flanger001 flanger001 commented Feb 4, 2020

Thanks @mperham!

@ritaly

This comment has been minimized.

Copy link

@ritaly ritaly commented Feb 23, 2020

@mperham, @pelargir looks like newrelic_rpm with version 6.9.0 is fixed and ready newrelic/rpm#313
Sidekiq 5.2.8 locked rack to 2.0.8, could be now unlocked?

@mperham

This comment has been minimized.

Copy link
Owner

@mperham mperham commented Feb 23, 2020

@ritaly Sidekiq 5.2.8 is locked to Rack 2.0.x which I think is reasonable. I don't want to support future Rack versions for a Sidekiq version which is in maintenance mode, given these issues with backwards compatibility. Sidekiq 6 will support Rack 2.1 and future versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.