Rack performance issue about honeybadger-ruby HOT 11 CLOSED

Hi Kevin,

Sorry for taking so long to get back to you. We really try to be more responsive than this.

I'll have the engineer in charge of the gem take a look at this and get back to you.

Best,
Starr

from honeybadger-ruby.

Hi. It's been another 2 weeks. Any update?

from honeybadger-ruby.

I'm sorry Kevin. I was moving over the past few weeks, so I'm a bit behind. Finally getting caught up. :)

You make an excellent point here. #34 looks good, but I'm wondering if we should change references to Honeybadger.context.clear! to simply Honeybadger.clear!. In the future, there is potential for #clear! to do more than clear just context. For example, we could take the same approach for tracking request data so that it's always available when calling Honeybadger.notify (someone had suggested that recently). What do you think?

from honeybadger-ruby.

No worries. I just was poking around in other issues and figured I'd prod :-)

I don't mind if you want to have two different methods. As written now, they're the same so I was just suggesting the simplification to cut out the intermediary work. If #34 is merged, the overhead here is reduced, but you're still potentially initializing a hash and dealing with thread local writes.

Having something like Honeybadger.clear_context! would allow you to just nil out the thread local. But granted you lose the chaining. I guess the way around that would be to revisit the "getter" doing initialization at all.

from honeybadger-ruby.

I'm going to go ahead and merge #34. We can actually take it one step further and not even initialize Thread.current[:honeybadger_context] if there is no hash passed to #context. I'll try making that change.

I think version 2.0 of the gem will take the same approach to set rack parameters and env, in which case it may be nice to create a separate object to wrap the thread storage. I'm still debating about keeping chaining intact - it's nice to have, but not sure the current implementation quite fits.

from honeybadger-ruby.

Not initializing the thread local sounds great. I didn’t go that far because I wasn’t sure if there were places where a non-nil object was expected and I wasn’t able to get the test suite running. But if that can be an avoided, that’d be fantastic. While these seem like small optimizations, they add up in long-running processes since that middleware is called on every single request to an app.

From: joshuap
Sent: ‎Wednesday‎, ‎July‎ ‎10‎, ‎2013 ‎11‎:‎11‎ ‎PM
To: honeybadger-io/honeybadger-ruby
Cc: Kevin Menard

I'm going to go ahead and merge #34. We can actually take it one step further and not even initialize Thread.current[:honeybadger_context] if there is no hash passed to #context. I'll try making that change.

I think version 2.0 of the gem will take the same approach to set rack parameters and env, in which case it may be nice to create a separate object to wrap the thread storage. I'm still debating about keeping chaining intact - it's nice to have, but not sure the current implementation quite fits.

—
Reply to this email directly or view it on GitHub.

from honeybadger-ruby.

Agreed! I added the change on #34, check it out if you want. I'll close this one, but may revisit later for v2.

from honeybadger-ruby.

Sorry, I'm confused on the resolution status here. Proposed API changes aside, you're still potentially creating a hash that you're storing in the thread local just to nil it out immediately after. If already storing context this isn't an issue, but if not, it's creating similar unnecessary object churn like #34. That was ultimately what I wanted to see addressed, not the API.

from honeybadger-ruby.

See my commit here: 6346c28

from honeybadger-ruby.

That's indeed the part I missed. Thanks a lot for taking care of this.

from honeybadger-ruby.

np, thanks for bringing it to my attention! :)

from honeybadger-ruby.