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

Agency: telemetry endpoint returns a success in case of failures #462

Closed
Retzoh opened this issue Mar 4, 2020 · 6 comments · Fixed by #461
Closed

Agency: telemetry endpoint returns a success in case of failures #462

Retzoh opened this issue Mar 4, 2020 · 6 comments · Fixed by #461
Labels
Agency Specific to the Agency API enhancement New feature or request
Milestone

Comments

@Retzoh
Copy link
Contributor

Retzoh commented Mar 4, 2020

Is your feature request related to a problem? Please describe.

The Agency spec defines responses for the telemetry endpoint as follows:

201 Success Response:

| Field     | Type                           | Field Description                                                                                       |
| --------- | ------------------------------ | ------------------------------------------------------------------------------------------------------- |
| `result`  | String                         | Responds with number of successfully written telemetry data points and total number of provided points. |
| `failures` | [Telemetry](#telemetry-data)[] | Array of failed telemetry for zero or more vehicles (empty if all successful).                          |

400 Failure Response:

| `error`         | `error_description`                  | `error_details`[]               |
| --------------- | ------------------------------------ | ------------------------------- |
| `bad_param`     | A validation error occurred.         | Array of parameters with errors |
| `invalid_data`  | None of the provided data was valid. |                                 |
| `missing_param` | A required parameter is missing.     | Array of missing parameters     |

This means that when the server fails to do what was expected of it (save new telemetries), it returns a success message.

Describe the solution you'd like

201 Success responses should only be used if all the telemetries were added.

I can see several alternatives if at least one telemetry could not be saved:

  1. The spec enforces that either all or none of the telemetries are accepted. When at least one telemetry causes an issue, the server rejects them all with a 200 response. The error description and error details explain which telemetries caused a problem and why.
  2. The server may accept some telemetries but reject others. If some were rejected, it returns a 200 providing the rejected telemetries in the error_details, with the reason they were rejected for.

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

For which spec is this feature being requested?

  • agency
@sarob sarob added Agency Specific to the Agency API enhancement New feature or request labels Mar 8, 2020
@jfh01 jfh01 added this to the 1.0.0 milestone Apr 9, 2020
@marie-x
Copy link
Collaborator

marie-x commented May 27, 2020

Flagging @avatarneil to investigate best-practice in bulk REST APIs

@marie-x
Copy link
Collaborator

marie-x commented May 27, 2020

I also think that solution 1 is too restrictive. Good telemetry should always be accepted IMHO.

@schnuerle
Copy link
Member

@Retzoh What do you think about the status of this issue and its relation to #461?

@Retzoh
Copy link
Contributor Author

Retzoh commented Jun 23, 2020

@schnuerle We haven't had time to discuss this issue in the working group. I will try to discuss this on Thursday, but I think it is low priority and could be pushed to 1.1.0 if there are more important matters to discuss.

However, If we do push it to 1.1.0, I think we should merge #461 (or any other specific response format) to have something specified in the spec and make the response machine-readable.

@Retzoh Retzoh linked a pull request Jun 25, 2020 that will close this issue
@schnuerle
Copy link
Member

@Retzoh what do you think is the next steps here based on the call Thursday, and now that #461 is merged?

@Retzoh
Copy link
Contributor Author

Retzoh commented Jun 29, 2020

@schnuerle I updated #461 to solve this issue. Since #451 was merged, I'm closing the issue.

@Retzoh Retzoh closed this as completed Jun 29, 2020
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants