Skip to content

Fix NoMethodError / Make session_tracking check consistent#2269

Merged
st0012 merged 2 commits intogetsentry:masterfrom
knapo:fix-determining-session_tracking
Mar 14, 2024
Merged

Fix NoMethodError / Make session_tracking check consistent#2269
st0012 merged 2 commits intogetsentry:masterfrom
knapo:fix-determining-session_tracking

Conversation

@knapo
Copy link
Copy Markdown
Contributor

@knapo knapo commented Mar 14, 2024

Description

#2245 (cc @st0012 ) introduced changes around checking if session_tracking is enabled or not.. However, it wasn't updated in all necessary places and make it inconsistent within the codebase. As result, when not enabled_in_current_env? we're getting a following error:

     NoMethodError:
       undefined method `add_session' for nil:NilClass
     # <REDACTED>/gems/sentry-ruby-5.17.0/lib/sentry/hub.rb:244:in `end_session'
     # <REDACTED>/gems/sentry-ruby-5.17.0/lib/sentry/hub.rb:253:in `with_session_tracking'
     # <REDACTED>/gems/sentry-ruby-5.17.0/lib/sentry-ruby.rb:403:in `with_session_tracking'

Affected version: sentry-ruby 5.17.0

@knapo knapo changed the title Make session_tracking check consistent Fix NoMethodError / Make session_tracking check consistent Mar 14, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 14, 2024

Codecov Report

Merging #2269 (9a84215) into master (b05afd9) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2269   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files         112      112           
  Lines        4155     4155           
=======================================
  Hits         4056     4056           
  Misses         99       99           
Components Coverage Δ
sentry-ruby 98.30% <100.00%> (ø)
sentry-rails 95.05% <ø> (ø)
sentry-sidekiq 94.70% <ø> (ø)
sentry-resque 92.30% <ø> (ø)
sentry-delayed_job 95.60% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
sentry-ruby/lib/sentry/hub.rb 99.38% <100.00%> (ø)

Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry for the bug and thanks for the fix!

@st0012 st0012 merged commit 8cfc110 into getsentry:master Mar 14, 2024
@knapo knapo deleted the fix-determining-session_tracking branch March 14, 2024 15:09
@ixti

This comment was marked as resolved.

@ixti
Copy link
Copy Markdown
Contributor

ixti commented Mar 17, 2024

Figured out the reason for #2269 (comment)

It happens when we do in specs:

require "sentry/test_helper"

RSpec.configure do |config|
  config.include Sentry::TestHelper, sentry_test: true
  config.before(sentry_test: true) { setup_sentry_test }
  config.after(sentry_test: true) { teardown_sentry_test }
end

@ixti
Copy link
Copy Markdown
Contributor

ixti commented Mar 17, 2024

Okay. Found how to reproduce the error:

Sentry.init do |config|
  # ...

  config.enabled_environments = %w[staging production]

  # ...
end

Then using in specs:

require "sentry/test_helper"

RSpec.configure do |config|
  config.include Sentry::TestHelper
end

RSpec.describe "..." do
  before { setup_sentry_test }
  after { teardown_sentry_test }

  # ...
end

setup_sentry_test injects "test" into environments

ixti added a commit to ixti/sentry-ruby that referenced this pull request Mar 17, 2024
Ensure enabled_environments contains test environment without mutating
original list.

See: getsentry#2269 (comment)
ixti added a commit to ixti/sentry-ruby that referenced this pull request Mar 22, 2024
Ensure enabled_environments contains test environment without mutating
original list.

See: getsentry#2269 (comment)
@Vagab Vagab mentioned this pull request Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants