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

Cannot remove breadcrumbs from huge envelope when config.async #1757

Closed
hogelog opened this issue Mar 11, 2022 · 5 comments · Fixed by #1758
Closed

Cannot remove breadcrumbs from huge envelope when config.async #1757

hogelog opened this issue Mar 11, 2022 · 5 comments · Fixed by #1758
Assignees
Projects
Milestone

Comments

@hogelog
Copy link

hogelog commented Mar 11, 2022

Issue Description

sentry-ruby 5.2.0 introduce envelope size check logic.
#1747

This logic check envelope size, and remove breadcrumbs if payload is too huge.
https://github.com/getsentry/sentry-ruby/blob/5.2.0/sentry-ruby/lib/sentry/transport.rb#L75-L78

But this breadcrumbs remove process does not work with config.async option.
item.payload has JSON-like string key when config.async is enabled.
https://github.com/getsentry/sentry-ruby/blob/5.2.0/sentry-ruby/lib/sentry/client.rb#L166-L168

Reproduction Steps

  • Start some kind of HTTP server port 8080.
    • example: docker run --rm -p 8080:80 ealen/echo-server
  • Run below ruby script.
require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "sentry-ruby", "5.2.0"
end

require "sentry-ruby"

Sentry.init do |config|
  config.dsn = 'http://foo@localhost:8080/1'
  config.async = lambda {|event| Sentry.send_event(event) } if ENV["ASYNC"]
  config.logger = Logger.new(STDOUT)
end

100.times.each { Sentry.add_breadcrumb(Sentry::Breadcrumb.new(message: "breadcrumb" * 1000)) }
Sentry.capture_message("large error message!"*10000)

Expected Behavior

Removing breadcrumbs logic work fine when config.async is enabled or not.

$ ruby app.rb 
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Using bundler 2.4.0.dev
Using concurrent-ruby 1.1.9
Using sentry-ruby-core 5.2.0
Using sentry-ruby 5.2.0
D, [2022-03-12T02:09:28.542692 #90042] DEBUG -- sentry: Sentry HTTP Transport will connect to http://localhost:8080
D, [2022-03-12T02:09:28.542749 #90042] DEBUG -- sentry: initialized a background worker with 10 threads
D, [2022-03-12T02:09:28.542776 #90042] DEBUG -- sentry: [Sessions] Sessions won't be captured without a valid release
I, [2022-03-12T02:09:28.544683 #90042]  INFO -- sentry: [Transport] Sending envelope with items [event] 7055984887fb46129ee69ad74ab11c00 to Sentry
$ ASYNC=1 ruby app.rb 
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Using bundler 2.4.0.dev
Using concurrent-ruby 1.1.9
Using sentry-ruby-core 5.2.0
Using sentry-ruby 5.2.0
D, [2022-03-12T02:09:30.516456 #90055] DEBUG -- sentry: Sentry HTTP Transport will connect to http://localhost:8080
D, [2022-03-12T02:09:30.516517 #90055] DEBUG -- sentry: config.async is set, BackgroundWorker is disabled
D, [2022-03-12T02:09:30.516529 #90055] DEBUG -- sentry: [Sessions] Sessions won't be captured without a valid release
D, [2022-03-12T02:09:30.521499 #90055] INFO -- sentry: [Transport] Sending envelope with items [event] ... to Sentry

Actual Behavior

Removing breadcrumbs logic does not work when config.async is enabled.

$ ruby app.rb 
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Using bundler 2.4.0.dev
Using concurrent-ruby 1.1.9
Using sentry-ruby-core 5.2.0
Using sentry-ruby 5.2.0
D, [2022-03-12T02:09:28.542692 #90042] DEBUG -- sentry: Sentry HTTP Transport will connect to http://localhost:8080
D, [2022-03-12T02:09:28.542749 #90042] DEBUG -- sentry: initialized a background worker with 10 threads
D, [2022-03-12T02:09:28.542776 #90042] DEBUG -- sentry: [Sessions] Sessions won't be captured without a valid release
I, [2022-03-12T02:09:28.544683 #90042]  INFO -- sentry: [Transport] Sending envelope with items [event] 7055984887fb46129ee69ad74ab11c00 to Sentry
$ ASYNC=1 ruby app.rb 
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Using bundler 2.4.0.dev
Using concurrent-ruby 1.1.9
Using sentry-ruby-core 5.2.0
Using sentry-ruby 5.2.0
D, [2022-03-12T02:09:30.516456 #90055] DEBUG -- sentry: Sentry HTTP Transport will connect to http://localhost:8080
D, [2022-03-12T02:09:30.516517 #90055] DEBUG -- sentry: config.async is set, BackgroundWorker is disabled
D, [2022-03-12T02:09:30.516529 #90055] DEBUG -- sentry: [Sessions] Sessions won't be captured without a valid release
D, [2022-03-12T02:09:30.521499 #90055] DEBUG -- sentry: Envelope item [event] is still oversized without breadcrumbs: {event_id: 34, level: 7, timestamp: 22, environment: 13, server_name: 15, modules: 98, message: 8195, user: 2, tags: 2, contexts: 388, extra: 2, fingerprint: 2, breadcrumbs: 828212, platform: 6, sdk: 40, threads: 1688}

Ruby Version

2.7.4

SDK Version

5.2.0

Integration and Its Version

No response

Sentry Config

No response

@st0012
Copy link
Collaborator

st0012 commented Mar 12, 2022

@hogelog thanks for reporting this, I've attached a PR for it.
and using config.async is actually not recommended. I've detailed the reasons in #1522 and please let me know your opinion on it 🙂

@hogelog
Copy link
Author

hogelog commented Mar 13, 2022

Thanks for the quick response!

I'm developing rails app that uses config.async with Sidekiq, but I will disable this config and use background worker or send synchronously.

It is very clear that the config.async feature incurs extra costs for sentry-ruby maintenance (just like this bug). I agree with removing this feature.

I will detailed comment to config.async on #1522.

The active maintenance of the sentry-ruby gem is very helpful, thank you.

5.x automation moved this from To do to Done Mar 14, 2022
@st0012 st0012 modified the milestones: 5.3.0, 5.2.1 Mar 18, 2022
@helmiattastify
Copy link

hi @st0012 is there any way to remove log ??

[Transport] Sending envelope with items [transaction] 29382938kjaskjdkajkdjasd to Sentry

@st0012
Copy link
Collaborator

st0012 commented Sep 9, 2022

You can always customize the SDK's log level by:

config.logger = Logger.new(STDOUT)
config.logger.level = Logger::WARN

@phyzical
Copy link

@st0012 sorry for bringing this back to life, but i found that making this change in the config block for sentry would update the overall apps logger to WARN, which means we loose things like route logs.

is there anyway to tell sentry not to notify that it is sending the envelopes besides at the logger level specifically?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5.x
Done
Development

Successfully merging a pull request may close this issue.

4 participants