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: add unregistered error #565

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

Retzoh
Copy link
Contributor

@Retzoh Retzoh commented Aug 24, 2020

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

As an agency, when receiving a batch of telemetry data including unregistered devices, there is currently no official way to let the provider know why the corresponding telemetries were rejected. This makes it complicated for the provider to automate the detection and resolution of this problem.

Describe the solution you'd like

Adapt the unregistered error existing in the Agency/event endpoint to Agency/telemetry.

Is this a breaking change

No, not breaking : we are just adding a new error type.

Impacted Spec

For which spec is this feature being requested?

  • agency

Describe alternatives you've considered

The current workaround we are using is to return a bad_param error on parameter device_id, Listing the unregistered ids in the error description: A validation error occured. Unregistered devices: UUID1, UUID2, etc..

Additional context

Add any other context or screenshots about the feature request here.

@Retzoh Retzoh requested a review from a team August 24, 2020 12:14
@Retzoh Retzoh requested a review from a team as a code owner August 24, 2020 12:14
@viestat
Copy link
Contributor

viestat commented Aug 24, 2020

Really looking forward for this!
It makes everything much simpler. As the consumer of the Agency endpoints it helps enormously to not only know what made a batch fail but specifically which vehicles are not registered. Then it is possible to trigger a register request for any vehicle present in that error as soon as this error is received.

@schnuerle schnuerle added the Agency Specific to the Agency API label Aug 24, 2020
@schnuerle schnuerle added this to the 1.1.0 milestone Aug 24, 2020
@marie-x
Copy link
Collaborator

marie-x commented Aug 24, 2020

This is already in the mds-core implementation. Will add to spec.

@marie-x marie-x self-assigned this Aug 24, 2020
@schnuerle
Copy link
Member

@schnuerle
Copy link
Member

From the call today @karcass will double check but it looks good and can be merged to dev.

Copy link
Collaborator

@marie-x marie-x left a comment

Choose a reason for hiding this comment

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

Ah, I see where this diverges from the mds-core implementation. unregistered shows up in the failures field and does not necessarily force a 400. What will show up in that case is the failures array will contain one entry per failure of the form

device_id ${telemetry.device_id}: not found

So I would change the PR to describe this as a possible failure entry. A bad device should not prevent the receipt of valid telemetry data.

What do you think about this counter-proposal?

@schnuerle
Copy link
Member

@karcass are you waiting on @Retzoh to reply here on your counter proposal? @Retzoh what do you think so we can merge this PR?

@marie-x
Copy link
Collaborator

marie-x commented Nov 18, 2020

@schnuerle Yes, I'm interested in what @Retzoh thinks about my counter.

@Retzoh
Copy link
Contributor Author

Retzoh commented Nov 23, 2020

Hi @karcass , sorry, I missed the initial ping.

I think it is dangerous to return a success code and rely on text-processing of the response payload to handle any errors.
To me, any error should be really straightforward.

Additionally, it seems more robust to me to just reject a invalid batch of telemetries until it is valid and pick-up the ingestion from there, rather than moving on and rely on tracking specific errors until resolved. With the second solution, it seems too easy both for the provider and the city to forget the errors and never handle them.

It would be interesting to have some feedback from providers on this. @viestat do you have an opinion ?

@schnuerle
Copy link
Member

Tagging some more providers who may have a technical opinion on this: @dirkdk @bhandzo.

@schnuerle
Copy link
Member

Do you think you can reach some consensus on this here in the GitHub PR in the next week so these changes can be added to the 1.1.0 release?

@dirkdk
Copy link
Contributor

dirkdk commented Dec 11, 2020

so the 2 alternatives are:

  1. return 400 HTTP error code and error with key failures that contains an array of unregistered vehicle ids
  2. return 200 HTTP success code but still a failures key that contains an array of unregistered vehicle ids.

For 1), for me it is implied that no data, even from already registered vehicles is processed
For 2), for me it is implied that correct data from registered vehicles is indeed processed but not for unregistered vehicles

IMHO, the solutions are very similar. I would opt for 1), as it is more explicit and its is clear that no data at all is processed and the client will need to correct their mistake and then retry.

@viestat
Copy link
Contributor

viestat commented Dec 14, 2020

In my opinion as the consumer of the Agency endpoints, I agree with what @Retzoh says about having to rely on the text response to identify errors.

Regarding @karcass 's counter proposal, if the endpoint takes batches as a whole it really does not make any sense to me to then treat individual items in the batch selectively. Either all the items that make up a batch are correct (and registered) in orderer to be processed or the batch is rejected and the reasons why it was rejected are made evident via the appropriate error (essentially what @dirkdk proposed above as option #1).

Doing this relieves the load on both sides (the consumer and the provider of the API) as it enables for a simpler retry flow and error handling.

Copy link
Collaborator

@marie-x marie-x left a comment

Choose a reason for hiding this comment

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

Sorry, I misread this diff originally. Is fine!

Copy link
Member

@schnuerle schnuerle left a comment

Choose a reason for hiding this comment

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

Looks good thanks for the conversations and concensus.

@schnuerle schnuerle merged commit 749dd22 into dev Dec 14, 2020
@schnuerle schnuerle deleted the retzoh/agency/telemetry-unregistered branch October 29, 2021 16:09
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants