rollbar / rollbar-gem

Exception tracking and logging from Ruby to Rollbar
https://docs.rollbar.com/docs/ruby
MIT License
446 stars 280 forks source link

Upgrading Sidekiq to 6.4 causes a warning #1077

Open gingerlime opened 2 years ago

gingerlime commented 2 years ago
Job arguments to Rollbar::Delay::Sidekiq do not serialize to JSON safely. This will raise an error in
Sidekiq 7.0. See https://github.com/mperham/sidekiq/wiki/Best-Practices or raise an error today
by calling `Sidekiq.strict_args!` during Sidekiq initialization.

See https://github.com/mperham/sidekiq/blob/main/Changes.md#640

gkampjes commented 2 years ago

I came across this same problem.

Setting config.async_json_payload = true fixed it for me.

isaacbowen commented 2 years ago

Yep, this just bit me. We started using Sidekiq.strict_args!, and didn't immediately realize that we'd lost our Rollbar error reporting entirely.

Thanks for the tip, @gkampjes! :)

I think it's reasonable for this gem to be proactive about this, because as of right now using Sidekiq.strict_args! without async_json_payload results in Rollbar dropping everything.

cc @mperham

waltjones commented 2 years ago

Using async_json_payload has better performance because the payload is only serialized once. Also if the payload needs to be truncated, this is done before adding the sidekiq queue, so it improves sidekiq ops as well.

On the next major version of the gem, async_json_payload will become the default or only behavior. The current default is preserved now for back compatibility.

thbar commented 2 years ago

Thanks everyone for the report!

I think it's reasonable for this gem to be proactive about this, because as of right now using Sidekiq.strict_args! without async_json_payload results in Rollbar dropping everything.

Indeed, an error during tests or anything else to warn people would be good here.

I have a staging server with upgrades at the moment, and "everything was going smoothly", except I wasn't aware of that.

@waltjones is there a way to fix this without necessarily going async for now, to your knowledge? (my client would prefer to have some time to migrate to async) Thanks!

choubacha commented 1 year ago

So, we just enabled this while upgrading sidekiq but now we are getting a stack overflow during what appears to be the json deserialization process. I don't know the original offending line, however, because the trace is truncated in the logs.

waltjones commented 1 year ago

@choubacha You're seeing this with the async_json_payload flag enabled?

choubacha commented 1 year ago

Yup. I suspect the issue maybe with the data that’s passed in but I cannot source the location of the overflow because the logs are truncated 😦

jakub-novvak commented 1 year ago

Just FYI if you use sidekiq 7 and follow the rollbar documentation to integrate it with sidekiq it just makes it not working at all. I came across this issue using sidekiq 7 and it was failing for me with just this log:

INFO -- : [Rollbar] Scheduling item
E, [2023-03-22T11:47:33.941329 #41] ERROR -- : [Rollbar] Async handler failed, and there are no failover handlers configured. See the docs for "failover_handlers"
I, [2023-03-22T11:47:33.941399 #41]  INFO -- : [Rollbar] Details: https://rollbar.com/instance/uuid?uuid=79660ce3-755f-438c-aeb6-070cbbe19b4e (only available if report was successful)

I was not getting why it was happening but it might look as if rollbar is not working at all with sidekiq 7 since it raises an error when the payload is not serialized correctly. It's a bit confusing as the docs don't mention it at all, but after enabling async_json_payload flag it started working properly.

waltjones commented 1 year ago

the docs don't mention it at all

@jakub-novvak thanks for pointing this out. I've updated the doc. https://docs.rollbar.com/docs/sidekiq-integration