Capture sessions automatically by default #523

Cawllec · 2019-01-11T16:04:43Z

Goal

Enable automatic session capture by default, and more strictly enforce endpoint setting.

…guration is complete

Cawllec · 2019-01-14T13:54:56Z

.rubocop_todo.yml

@@ -305,6 +305,7 @@ Metrics/ModuleLength:
  Max: 125
  Exclude:
    - 'lib/bugsnag/helpers.rb'
+    - 'lib/bugsnag.rb'


Note - this exclusion was originally approved in the breadcrumbs/base branch.

tobyhs · 2019-01-14T18:59:50Z

features/fixtures/docker-compose.yml

@@ -117,6 +117,7 @@ services:
      - BUGSNAG_SESSION_ENDPOINT
      - BUGSNAG_TIMEOUT
      - CALLBACK_INITIATOR
+      - USE_DEFAULT_AUTO_CAPTURE_SESSIONS


I think I'm having trouble finding where this is set (from searching for USE_DEFAULT_AUTO_CAPTURE_SESSIONS in this pull request diff)

This is my bad, I'd removed some tests temporarily and didn't add them back! Pushing up the rest of the tests now.

tobyhs · 2019-01-14T19:00:52Z

features/fixtures/rails3/app/app/controllers/session_tracking_controller.rb

+    render json: {}
+  end
+
+  def hundred


I would name this many or multiple or something like that so the name still makes sense if you change the number of sessions started.

tobyhs · 2019-01-14T19:08:49Z

lib/bugsnag/configuration.rb

+
+    ##
+    # @return [Boolean] whether any sessions types will be delivered
+    attr_reader :send_sessions


Having a boolean setting/option/attribute with the same name as a public method in SessionTracker (send_sessions) might be slightly confusing. It might be preferable to rename this option to something like sessions_enabled or session_tracking_enabled instead.

spec/bugsnag_spec.rb

tobyhs · 2019-01-14T19:19:04Z

spec/session_tracker_spec.rb

+    expect(Bugsnag.configuration.send_sessions).to eq(false)
+    expect(Bugsnag.session_tracker.session_counts.size).to eq(0)
+    Bugsnag.start_session
+    # expect(Bugsnag.session_tracker.session_counts.size).to eq(0)


Did you intend to uncomment this out?

tobyhs · 2019-01-14T19:25:15Z

spec/session_tracker_spec.rb

+# Enable reset of session_counts between each example
+module Bugsnag
+  class SessionTracker
+    attr_accessor :session_counts


I think you can avoid opening/monkey-patching this class by having a before block call Bugsnag.session_tracker.send_sessions to flush session_counts, although you will probably need to set the delivery method to synchronous and clear queue right after the send_sessions call.

To be honest, as long as the monkey-patching won't interfere with the tests I don't have a problem with it.

A risk of monkey patching like this is that someone might introduce app code that uses the extra behavior (Bugsnag::SessionTracker#session_counts= in this case) and rely on the unit tests to make sure things work, which will be not as accurate as the tests introduce an extra method while the app code doesn't have this extra method.

Alternatively, you can do something similar to the global before(:each) block in spec_helper.rb and have Bugsnag.instance_variable_set(:@session_tracker, Bugsnag::SessionTracker.new) in a before block in this file (which has the benefit that anything in Bugsnag.session_tracker is reset if more instance variables are added to Bugsnag::SessionTracker later).

That makes sense, and I quite like that last suggestion, more so than relying on send_sessions.

Shouldn't you remove this monkey patch and the require 'concurrent' now that you're using Bugsnag.instance_variable_set(:@session_tracker, Bugsnag::SessionTracker.new)?

Also, I would put it in a before(:each) instead, then have an after(:all) so you have something fresh before the first test in this file also.

tobyhs · 2019-01-14T19:33:58Z

features/rails_features/auto_capture_sessions.feature

  Then I should receive a request
  And the request is a valid for the session tracking API
  And the request used the "Ruby Bugsnag Notifier" notifier
  And the "Bugsnag-API-Key" header equals "a35a2a72bd230ac0aa0f52715bbdc6aa"
  And the sessionCount "startedAt" is a timestamp
+  And the sessionCount "sessionsStarted" equals 100


Is this a little prone to non-deterministic failures because the 100 sessions could span across 2 different minutes?

features/rails_features/auto_capture_sessions.feature

…to 'enable_sessions' to avoid confusion

tobyhs

a couple minor comments, otherwise lgtm, but I disagree with the monkey patch in session_tracker_spec.rb

tobyhs · 2019-01-15T19:08:39Z

features/steps/ruby_notifier_steps.rb

+Then(/^the total sessionStarted count equals (\d+)(?: for request (\d+))?$/) do |value, request_index|
+  session_counts = read_key_path(find_request(request_index)[:body], "sessionCounts")
+  total_count = session_counts.inject(0) { |count, session| count += session["sessionsStarted"] }
+  assert_equal(total_count, value)


Flip these arguments ((value, total_count)); the first parameter is expected and the second is actual.

tobyhs · 2019-01-15T19:25:54Z

spec/session_tracker_spec.rb

+# Enable reset of session_counts between each example
+module Bugsnag
+  class SessionTracker
+    attr_accessor :session_counts


A risk of monkey patching like this is that someone might introduce app code that uses the extra behavior (Bugsnag::SessionTracker#session_counts= in this case) and rely on the unit tests to make sure things work, which will be not as accurate as the tests introduce an extra method while the app code doesn't have this extra method.

Alternatively, you can do something similar to the global before(:each) block in spec_helper.rb and have Bugsnag.instance_variable_set(:@session_tracker, Bugsnag::SessionTracker.new) in a before block in this file (which has the benefit that anything in Bugsnag.session_tracker is reset if more instance variables are added to Bugsnag::SessionTracker later).

tobyhs · 2019-01-15T19:26:46Z

spec/configuration_spec.rb


-    it "should have sensible defaults for session tracking" do
-      expect(subject.session_endpoint).to eq("https://sessions.bugsnag.com")
+  describe "disable_sessions" do


#disable_sessions (missing #)

Cawllec added 15 commits January 4, 2019 16:17

Auto Session Tracking: Update endpoint setting and session sending logic

9d62426

Auto session tracking: Fixed configuration syntax, simple test cases

bc25018

Auto session tracking: Minor fixes

dd0df56

Auto session tracking: fix session_tracker tests

d3eb178

Auto session tracking: Add configuration tests for new options

f58a8a7

Auto session tracking: Add send_sessions test

59fd4e6

Auto session tracking: Style improvements

58425d7

Auto session tracking: Ensure endpoint setup is validated after confi…

6b72fda

…guration is complete

Auto session tracking: Ensure logging occurs through configuration

dfce288

Auto session tracking: Add tests for Bugsnag module changes

3937981

Auto session tracking: Spelling corrections

ba77b57

Auto session tracking: Style fixes

ad4d527

Auto session tracking: Fix order-dependent & flaky tests

55f808a

Auto session tracking: Updated maze tests to reflect new defaults

e84ddb6

Merge branch 'master' into auto-session-tracking

63df0d9

Cawllec commented Jan 14, 2019

View reviewed changes

tobyhs reviewed Jan 14, 2019

View reviewed changes

features/rails_features/auto_capture_sessions.feature Show resolved Hide resolved

Cawllec added 3 commits January 15, 2019 11:20

Auto session tracking: Address test feedback

c1b0bbb

Auto session tracking: Renamed 'Configuration#send_sessions' boolean …

1a22e59

…to 'enable_sessions' to avoid confusion

Auto session tracking: Minor spec improvements

ad8da27

tobyhs approved these changes Jan 15, 2019

View reviewed changes

Auto session tracking: Test improvements

8f6d53f

Cawllec merged commit d8aaa21 into master Jan 16, 2019

imjoehaines deleted the auto-session-tracking branch June 16, 2021 08:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Capture sessions automatically by default #523

Capture sessions automatically by default #523

Cawllec commented Jan 11, 2019 •

edited

Loading

Cawllec Jan 14, 2019

tobyhs Jan 14, 2019

Cawllec Jan 15, 2019

tobyhs Jan 14, 2019

tobyhs Jan 14, 2019

tobyhs Jan 14, 2019

tobyhs Jan 14, 2019

Cawllec Jan 15, 2019

tobyhs Jan 15, 2019

Cawllec Jan 16, 2019

tobyhs Jan 16, 2019

tobyhs Jan 14, 2019

tobyhs left a comment

tobyhs Jan 15, 2019

tobyhs Jan 15, 2019

tobyhs Jan 15, 2019

Capture sessions automatically by default #523

Capture sessions automatically by default #523

Conversation

Cawllec commented Jan 11, 2019 • edited Loading

Goal

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobyhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cawllec commented Jan 11, 2019 •

edited

Loading