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

Add lost property to vehicles #32

Open
faultyserver opened this issue Sep 25, 2016 · 7 comments
Open

Add lost property to vehicles #32

faultyserver opened this issue Sep 25, 2016 · 7 comments
Milestone

Comments

@faultyserver
Copy link
Member

faultyserver commented Sep 25, 2016

In the course of an agency's operation, there may be times where a Vehicle is taken out of commission while it is traveling on a Route. I've had a personal experience where a bus caught fire and was unable to continue it's trip. As one of 2 buses on the 25 minute loop, this caused major delays for the route, but was completely ignored by existing transit applications at the time.

Most applications continued reporting the original scheduled stop times for the route (even though the bus that would make those stops was not in service), while others just showed increasing wait times for the bus.

The idea of the lost property of a Vehicle is an attempt to automatically determine situations like these and inform users of major delays as early as possible (rather than just accumulating wait times).

The implementation of this property will be through a Middleware class that adds it to the Vehicle model. The Middleware will also add a hidden last_moved_at property to preserve state through multiple update cycles. On every cycle where the latitude or longitude of the Vehicle changes, last_moved_at will be updated to Time.now. If last_moved_at exceeds a configurable threshold, then lost will be set to true for the Vehicle.

When the lost property is true, the Middleware will not proxy events for that vehicle up the rest of the stack. The Middleware will, however, send a lost event up the stack so that clients can respond accordingly. This middleware should come closely after the Agency in the stack to avoid any miscellaneous events relating to the Vehicle coming from other Middlewares.

Once the position of a Vehicle changes again, it's lost property will be set to false and normal proxying will resume.

Here's a quick implementation of the Middleware:

class Runaway < Shark::Middleware # Names subject to change
  on_install do
    Shark::Vehicle.attribute :lost, default: false, nilable: false, required: false
    Shark::Vehicle.attribute :last_moved_at, default: ->{ Time.now }, nilable: true, required: false
  end

  on 'vehicles', :update  do |event|
    vehicle = event.args.first

    # If the vehicle moved in this cycle, update `last_moved_at`
    vehicle.last_moved_at = Time.now if vehicle.position_changed?

    # Only fire the `lost` event once after losing the vehicle.
    if vehicle.last_moved_at > configuration.threshold && !vehicle.lost?
      vehicle.lost = true
      fire(Shark::Event.new(
        topic: vehicle.identifier,
        type: :lost,
        args: [vehicle],
        kwargs: {},
        originator: vehicle.identifier
      ))
    else
      vehicle.lost = false
      # Potentially fire a `found` event
    end

    # Only proxy the original event if the vehicle is not currently lost.
    fire(event) unless vehicle.lost?
  end
end
@elliottwilliams
Copy link
Member

elliottwilliams commented Sep 25, 2016 via email

@faultyserver
Copy link
Member Author

Correct. This is very much an extrapolation, so there are bound to be false positives.

Perhaps having the if vehicle.last_moved_at > configuration.threshold conditional be configurable would help this. Something like this, maybe:

require 'active_support/core_ext/numeric/time'

Runaway.configure do |config|
  config.threshold = 10.minutes

  config.lost_condition = Proc.new do |vehicle|
    vehicle.route ? (Time.now - vehicle.last_moved_at) > config.threshold : false
  end
end

In an effort to avoid over-configuration, I think having the default just be the time condition and defaulting to 10 minutes is sufficient for most cases. If we determine that it is not, and we have the ability to write a better heuristic, we can then add that in the configuration.

@faultyserver faultyserver added this to the v1.1 milestone Sep 25, 2016
@elliottwilliams
Copy link
Member

Yeah. I don't think configuration of the condition is necessary, my point is more that consumers of the event model ought to know that "lost" might not mean actually lost.

Another direction to take this: Can the lost condition be based on the vehicle not being stopped at its next_station on a timepoint? This entails calling timetable.next_visit(vehicle.station, vehicle.route) and only setting lost if Time.now is not between the returned ETA and ETD.

The downside to this would be a dependency on Timetable from a relatively low-level Shark middleware.

@faultyserver
Copy link
Member Author

I'm certainly open to suggestions on better lost conditions, but - from the way I'm reading it - I don't think what you described would work well, aside from the fact that it's adding a dependency.

What happens when a vehicle is 11 minutes behind schedule and gets stuck at a stop light? My understanding of your idea says that the vehicle would be lost, since Time.now ⊈ (ETA, ETD).

I do think that the "stationary for 10 minutes" conditional is rather naive, and would like to see it substituted. Am I misunderstanding your suggestion?

faultyserver added a commit that referenced this issue Sep 26, 2016
Objects now include Dirtiable, which tracks changes to attributes defined through `attr_accessor` and `attr_writer`. Changes can also be recorded manually using `dirty_attribute <name>, <old_value>, <new_value>`. Changes are stored until `clean!` is called, after which the object is no longer considered dirty, and tracking will start from a blank slate.

This allows components that have some dependence on state (the upcoming `LostNFound` middleware, for example. See #32) to be provided a set of changes, rather than having to maintain/determine that information themselves. The result is leaner components and a single source of truth about object dirtiness.

Example usage of Dirtiable:
  vehicle = Shark::Vehicle.new(name: '4003', latitude: 10, longitude: -10)
  vehicle.dirty?          #=> false
  vehicle.changes         #=> { }
  vehicle.latitude = 15
  vehicle.dirty?          #=> true
  vehicle.changes         #=> { latitude: [[10, 15]] }
  vehicle.name = '4004'
  vehicle.latitude = 10
  vehicle.longitude = -15
  vehicle.changes         #=> { name: [['4003', '4004']], latitude: [[10, 15], [15, 10]], longitude: [[-10, -15]] }
  vehicle.clean!
  vehicle.dirty?          #=> false
  vehicle.changes         #=> { }
@elliottwilliams
Copy link
Member

I should have added "additionally" to my explanation. I'm suggesting that the lost condition works like you described, with a n-minutes-without-movement threshold, with the additional condition that the vehicle isn't stopped because it is at a long timepoint. A vehicle shouldn't become lost because it's waiting for an upcoming departure time.

FWIW, I don't think CityBus has anything other than 5 minute timepoints. If you want to check for yourself, this awk script prints every nonequal ETA and ETD: awk -F , '$2 != "" && $2 != $3 { print $1,$2,$3}' stop_times.txt

@faultyserver
Copy link
Member Author

That makes a lot more sense. I'm not sure why I didn't assume you meant additionally.

However, I'm not sold on the benefits of using this heuristic with the cost of having RPCs in the middleware stack. I think there are still plenty of cases where buses would be wrongly considered "lost". The main one that comes to mind is when drivers leave the bus to take mandated breaks: the bus is technically active, and travelling on a route, but can often be delayed by 10+ minutes (I've seen one driver take 16 minutes to return to the bus).

My thoughts for now are to just have the time condition, and see how many lost events get fired in the course of a week or so. I think crafting a better heuristic will be much easier with some experiential data.

@elliottwilliams
Copy link
Member

Sure. I'm all for keeping things simple until we actually detect a problem here :)

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

No branches or pull requests

2 participants