pschinis / rails_same_site_cookie

Manages the new SameSite=None behavior for Rails apps that use cookie-based authentication for cross-domain requests
MIT License
107 stars 65 forks source link

Performance impact #5

Closed bennick closed 4 years ago

bennick commented 4 years ago

Overall great project. I had some special use cases so didn't use the gem directly but did use much of what you have in the middleware piece.

Once deployed we noticed a huge impact on CPU.

Screen Shot 2020-01-16 at 11 56 14 AM

The spike was right after our deployment, the downslope is from us adding more app servers, and the final drop was from a second deployment removing the high impact code.

I believe the impact is from the user_agent_parser gem, included in this project, parses a very large Yaml file on every request.

@pschinis I just wanted you to be aware of this. You should either remove the dependency on user_agent_parser or work in a caching scheme into the middleware.

We replaced most of the UserAgentChecker code with methods inspired by this gist. Basically removing the call to UserAgentParser#parse solved our performance issue.

pschinis commented 4 years ago

Thanks for noticing and investigating this. I'll try to update the gem to get rid of this dependency over the weekend!

sirneb commented 4 years ago

@pschinis Thanks for looking into this, I'm actually in the progress of evaluating this gem also. My current plan is also to evaluate solutions to work around this performance issue.

sirneb commented 4 years ago

Just so you know, instead of using UserAgentParser#parse, you can use UserAgentParser::Parser#parse which skips reloading the yml and other unnecessary processing more than once per process. This might be good enough to fix the performance issue.

# The parser database will be loaded and parsed on every call to
# UserAgentParser.parse. To avoid this, instantiate your own Parser instance.
parser = UserAgentParser::Parser.new
parser.parse 'Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.0;)'
=> #<UserAgentParser::UserAgent IE 9.0 (Windows Vista)>
parser.parse 'Opera/9.80 (Windows NT 5.1; U; ru) Presto/2.5.24 Version/10.53'
=> #<UserAgentParser::UserAgent Opera 10.53 (Windows XP)>

https://github.com/ua-parser/uap-ruby#example-usage

Unfortunately for me, I just realized that uap-ruby require Ruby >= 2.4 and we are still on 2.3.3. So I'll need to evaluate another solution.

pschinis commented 4 years ago

Just merged this pull request which fixes this issue basically how @sirneb suggested by using an instance of UserAgentParser instead of the static method, which avoids reloading the YML on every call.

Also @sirneb I'm using this gem with Ruby 2.3 so I think you should be able to get it to work. uap-ruby 2.5.1 still supports Ruby 2.3 and the gem only requires ~>2.5 so it should work. I did notice when I installed my bundle that bundler was trying to update my uap version to 2.5.2 and then failing out because that requires Ruby 2.4, but then I just ran it again and somehow it installed fine using 2.5.1. So if you're running into issues you should be able to lock your uap version to 2.5.1 and get it working.

KelseyDH commented 4 years ago

I'm interested to see benchmarks after this change!