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

Generic way for warnings/deprecations on API response #412

Open
soxofaan opened this issue Sep 13, 2021 · 14 comments · Fixed by #419
Open

Generic way for warnings/deprecations on API response #412

soxofaan opened this issue Sep 13, 2021 · 14 comments · Fixed by #419

Comments

@soxofaan
Copy link
Member

Use case: openEO platform aggregator implements /jobs listing the union of batch jobs across all underlying back-ends. At the moment the EODC back-end errors with something like "user ... does not exist and is not whitelisted", causing the whole aggregator response to fail too.
The workaround I'm currently working on is to skip the failed response of EODC and just list the jobs from other back-ends (e.g. VITO), which is better than returning nothing. It is however not ideal because user will not see that jobs are possibly missing due to a (temporary) issue at one back-end.

I think it would be good to have a somewhat generic way to indicate that there were (non-fatal) errors/warnings/deprecations/issues for a returned response.
For example:

  • Dedicated HTTP header, e.g. with at least a problem counter, or link to url with listing.
  • a dedicated link relation type to be used in "links" sections of JSON responses to link to url with listing.
@soxofaan
Copy link
Member Author

This feature would be handy for Open-EO/openeo-aggregator#18 (the generalization of OP's use case)

At the moment I'm most inclined to add a HTTP header to the response:

  • pretty generic for both back-end side and client side, doesn't require definition of new endpoints
  • doesn't require some kind of stateful managed storage at back-end-side. Note that it must work on all kind of endpoints, also non-authenticated discovery endpoints, and a backend probably doesn't want to store/manage warning logs for all these
  • health data is included in response so client doesn't have to do additional request for check health of a previous request

@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

I definitely see the use case. I'm wondering whether HTTP status code 206 ("Partial Content") instead of 200 ("OK") would work here? It sounds like it would fit in and I'd hope that clients would handle it like a 200, but still could make use of it if they want to communicate that this was a partial result. But maybe a header is the better option as 206 is not allowed in the current spec and would be somewhat breaking.

Nevertheless, this seems a bit out of scope for the core API spec and would more belong into an extension that handles federation aspects (which may also include additions such as "openeo:backends" etc.).

@soxofaan
Copy link
Member Author

would more belong into an extension that handles federation aspects

I see it broader than just a "partial-federation-result" thing.
Also, the feature only makes sense if client libs support it, and the client side is usually unaware that it is working in a federated context.

@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

In a non-federated environment, I don't see much use in a partial response though?! What's the use case there?

The (very simple) federation extension I'm currently writing would add a federation flag (or a list of backends) to capabilities, so clients supporting federation aspects would know that they are connected to a federation.

Still, the 206 status code would either need to go into core, too, or we need to use HTTP headers as right now the core API only allows returning 200. So we may indeed go for a HTTP header, which is better wrt to extensibility.

@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

How should such a header be defined? Should it just be a flag or a list of back-ends missing (or contained) in the response?

Examples:

  1. in a federation with eodc, vito, wwu and eurac and wwu and eurac being offline: Federation-Missing-Backend: wwu, eurac?
  2. Federation-Backend-Offline: true

@soxofaan
Copy link
Member Author

In this ticket, I'm aiming for something more generic than just the Open-EO/openeo-aggregator#18 issue.
e.g. response headers like

X-OPENEO-WARNING: Collection listing is incomplete because backend "eodc" is down

X-OPENEO-WARNING: The UDP id differs from the process_graph_id URL parameter

@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

Hmm... I'm a bit hesitant here. Could this lead to bypassing the logging for processing requests?

PS: The X- prefix has been deprecated in general.

@soxofaan
Copy link
Member Author

Could this lead to bypassing the logging for processing requests?

I think it can be complementary to (job) logs:

  • header based warnings: covers all endpoints, methods, response data types automatically, without additional roundtrips. Recommended to be no-nonsense and short
  • (job) logs: only for processing requests, requires requires dedicated /logs endpoints, requires non-generic client support. Recommended to be sufficiently verbose and information-rich

@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

Also, allowing this for all openEO endpoints in OpenAPI is quite cumbersome (and repetitive in the spec). Federation-Missing-Backend (or similar) would just be needed for the "list" endpoints (all GET), not all of them. Actually, we don't even need to use a header there...

I think you'd need to add the new header for non-GET requests to CORS, too, which makes it even more annoying to add.

@m-mohr m-mohr linked a pull request Oct 19, 2021 that will close this issue
@m-mohr
Copy link
Member

m-mohr commented Oct 19, 2021

Related PR: #419

@m-mohr
Copy link
Member

m-mohr commented Nov 2, 2022

There was a Warning header until 2022, but it is now deprecated:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Warning
Bummer...

@soxofaan
Copy link
Member Author

Feature that could help with communicating warnings/deprecations in certain situations is /validation endpoint.
The python client (since version 0.24) automatically calls /validation and visualizes issues as warnings (logging.warning style), both on sync execution and batch job creation requests. This gives a backend the opportunity to flag (non-fatal) problems.

@m-mohr
Copy link
Member

m-mohr commented Dec 11, 2023

Sounds like a good idea, although I don't like mixing warnings and errors in the client. Shouldn't the Python client use logging.error?

We could extend the validation output to contain a "warnings" array in addition to the "errors" array. Then they can be distunguised better.

@soxofaan
Copy link
Member Author

... mixing warnings and errors in the client. Shouldn't the Python client use logging.error?

Because validation support is somewhat experimental in some backends (VITO backend included) we didn't want to make this auto-validation feature a blocking thing: even if validation gives errors, we still let the sync evaluation or job creation go through. In that sense it is currently more informative, to get some real world testing of the validation feature.

In practice we even experience that users are now reporting these informative validation messages as real errors, even though their workflow is not actually blocked or failing at all. So in a way one could even argue that logging with level INFO is more appropriate at the moment. That being said, WARNING level is fine at the moment in my opinion.

We could extend the validation output to contain a "warnings" array in addition to the "errors" array. Then they can be distinguished better.

that would indeed be a good idea, and easy to add to the current schema

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

Successfully merging a pull request may close this issue.

2 participants