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

Fix NoMethodError / Make session_tracking check consistent #2269

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

knapo
Copy link
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
Copy link

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
Collaborator

@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
122 checks passed
@knapo knapo deleted the fix-determining-session_tracking branch March 14, 2024 15:09
@ixti

This comment was marked as resolved.

@ixti
Copy link

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

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)
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.

None yet

3 participants