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

Provider/Agency API alignment #796

Merged
merged 31 commits into from
Jan 19, 2023
Merged

Conversation

marie-x
Copy link
Collaborator

@marie-x marie-x commented Oct 26, 2022

Explain pull request

Agency and Provider have been converging with each iteration. Time to fully align data structures and harmonize the APIs.

Is this a breaking change

A breaking change would require consumers or implementors of the API to modify their code for it to continue to function (ex: renaming of a required field or the change in data type of an existing field). A non-breaking change would allow existing code to continue to function (ex: addition of an optional field or the creation of a new optional endpoint).

  • Yes, breaking

Impacted Spec

Which spec(s) will this pull request impact?

  • agency
  • provider

Additional context

Github issue: #759

Task force link: https://github.com/openmobilityfoundation/mobility-data-specification/wiki/Unification-WG-Task-Force

Design doc: https://docs.google.com/document/d/134DrPFjSpapW1auTT2M_BuL2E9tdO1At0yu6VII1cPA/edit#

@marie-x
Copy link
Collaborator Author

marie-x commented Oct 26, 2022

Still a fair amount to do - most changes are in the Provider doc, I'll come back to clean up Agency after we get a good look at the proposed Provider changes. Also there are a few open questions, denoted with (?) in the PR.

@schnuerle schnuerle linked an issue Oct 26, 2022 that may be closed by this pull request
@schnuerle schnuerle added Provider Specific to the Provider API Agency Specific to the Agency API Schema Implications for JSON Schema or OpenAPI State Machine Changes in the vehicle state events and state machine diagram labels Oct 26, 2022
@schnuerle schnuerle added this to the 2.0.0 milestone Oct 26, 2022
@pierre-bouffort
Copy link
Collaborator

Hello all, thanks a lot for the work that was put in this PR. I think it's is very good overall.

2 quick thoughts and comments:

  • I'm not sure I understand the interest in having two separate endpoints and separate query modes for the /events/recent and the /events/historical , it seems to add a layer of complexity. But maybe I'm missing something.

  • We are adding to the Provider API an endpoint /telemetry which allows to poll telemetry between two dates or according to a trip_id. That is a great addition. But I've noticed that the telemetry object does not contain a trip_id field. I expect this could make the cross-referencing of the telemetries pulled via this endpoint with the ones in the routes inside the Trip objects more complicated. Do you guys think we could add field for the trip_id as optional in the telemetry object?

@marie-x
Copy link
Collaborator Author

marie-x commented Oct 27, 2022

Good catches. I will explain the proposed recent/historical split at today's review. Omitting telemetry trip_id was an oversight which I have remedied.

| `battery_pct` | Float | Required if Applicable | Percent battery charge of device, expressed between 0 and 1 |
**`data` Payload:** `{ "vehicles": [] }`, an array of [Vehicle](vehicle) objects

(?) Open question: should we standardize on telemetry over GeoJSON Point? E.g. replace last_event_location with last_event_telemetry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Another open question: if we're dropping device data from events/trips and pointing people at /vehicles instead I think this will need some additional parametrization like the ability to get info on a specific device and/or the devices in use within a specific time range.

Copy link
Member

Choose a reason for hiding this comment

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

@alexdemisch
Copy link
Collaborator

The current SFMTA taxi spec has a telemetry API very similar to the Agency telemetry endpoint. The SFMTA spec includes a driver_status and vehicle_status field, which makes it much easier to analyze telemetry data.

The driver_status values are:

  • 1 (Starting Shift)
  • 2 (On Shift)
  • 3 (Ending Shift)

And the vehicle_status values are:

  • 1 (Off Duty)
  • 2 (Available)
  • 3 (Hired)

Some of the use cases are cross-checking whether we have received trip data in all cases where the vehicle was 'hired', distinguish between dead-heading and just off-duty driving, and analyze driver shifts, including (but not limited to) detecting cases of excessive driving without breaks

The PR has a trip_id, which would allow us to identify telemetry records that are related to a revenue trip, there’s not an easy way to identify other statuses. One way to achieve this would be to have last_vehicle_state and last_event_types fields a la the vehicles endpoint. I’m not sure how much of a lift this would be on the provider side, or if there are other ways to get at this that don’t involve trying to cross reference the telemetry and events streams.

@pierre-bouffort
Copy link
Collaborator

Read through this PR again, and it seems fine by me. I already have expressed my doubts about the split between historical and recent events in the Provider API, but this has already been discussed, and if I'm the only one with this concern, I will obviously align with the general opinion.
Thanks for your work here @marie-x :-)

@schnuerle
Copy link
Member

It would be great to get this out of draft and ready to merge to dev by Monday Dec 19.

@marie-x could you complete the open work and questions to the best of your ability, and leave inline comments or general comments for areas you weren't sure of?

There are a number of conflicts to be resolved now too.

Also please double check links across the spec that now should go to the data types or general information page, and check reference links which could be broken now.

data-types.md Outdated Show resolved Hide resolved
@schnuerle
Copy link
Member

Could you review #740 and leave a comment there if you think it's ok to merge with the work there?

Also there are some conflicts to resolve if you can because of merging some other work.

@pierre-bouffort
Copy link
Collaborator

Are still looking at adding the surface location type now that the unification PR is mature ?
#767

@schnuerle
Copy link
Member

Are still looking at adding the surface location type now that the unification PR is mature ? #767

Yes we can still work this in (and related ones) once #767 is merged to dev (still in progress). cc @marie-x

data-types.md Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@marie-x marie-x marked this pull request as ready for review January 19, 2023 12:25
@marie-x marie-x requested a review from a team as a code owner January 19, 2023 12:25
@marie-x marie-x changed the title [DRAFT] Provider/Agency API alignment Provider/Agency API alignment Jan 19, 2023
@schnuerle schnuerle merged commit 359a86c into dev Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agency Specific to the Agency API Provider Specific to the Provider API Schema Implications for JSON Schema or OpenAPI State Machine Changes in the vehicle state events and state machine diagram
Projects
None yet
6 participants