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

Session tracking fixes #412

Merged
merged 26 commits into from
Jan 9, 2018
Merged

Session tracking fixes #412

merged 26 commits into from
Jan 9, 2018

Conversation

Cawllec
Copy link
Contributor

@Cawllec Cawllec commented Jan 9, 2018

Fixes several issue from initial session-tracking release including:

  • Standardises API
  • Reduces number of spawned threads
  • Uses concurrent ruby for safer concurrency
  • Fixed breaking changes

@Cawllec Cawllec requested a review from snmaynard January 9, 2018 14:55
bugsnag.gemspec Outdated
@@ -18,4 +18,5 @@ Gem::Specification.new do |s|
]
s.require_paths = ["lib"]
s.required_ruby_version = '>= 1.9.2'
s.add_runtime_dependency 'concurrent-ruby', '~> 1.0.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably be ~> 1.0

@@ -3,13 +3,17 @@
require 'spec_helper'
require 'json'

module Bugsnag
class SessionTracker
attr_accessor :track_sessions
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was being used to access a variable to stop the delivery threads being spun up, but is obsolete now.

Bugsnag.session_tracker.create_session
Bugsnag.session_tracker.start_session
while Bugsnag.session_tracker.session_counts.size == 0
sleep(0.05)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these required?

conf.release_stage = "test_stage"
end
Bugsnag.session_tracker.create_session
Bugsnag.session_tracker.start_session
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just call Bugsnag.start_session

sessions = []
@session_counts.each do |min, count|
counts = @session_counts.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

You actually want

counts = @session_counts
@session_counts = Concurrent::Hash.new(0)
counts.each ...

With the current implementation there is still a race condition.

end

def deliver(session_counts)
if session_counts.length == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

be consistent about using @session_counts or session_counts in this file. I would use the member variable.

CHANGELOG.md Outdated
@@ -6,7 +6,7 @@ Changelog
### Enhancements

* Adds support for tracking sessions and crash rate by setting the configuration option `configuration.track_sessions` to `true`.
Sessions can be manually created using `Bugsnag.start_session`, and manually delivered using `Bugsnag.send_sessions`.
Sessions can be manually created using `Bugsnag.create_session`, and manually delivered using `Bugsnag.send_sessions`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't mention the send_sessions thing. Ideally that shouldn't even be public imo. It doesn't look like it's defined on Bugsnag anyway. The function should be start_session not create_session

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's the old changelog. I'll fix the copy.

@Cawllec Cawllec merged commit 999f364 into master Jan 9, 2018
@Cawllec Cawllec deleted the session-tracking-fixes branch February 7, 2018 11:25
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.

2 participants