Enhancing debug information for HttpDataSource transfers

[zub4t]

Feature Request

Currently, when the openPartStream method on HttpDataSource is called, if the transfer fails the reason for this failure is not logged. Providing information about why a transfer fails or when it succeeds would improve the maintenance of the connector.

Which Areas Would Be Affected?

transfer

Why Is the Feature Desired?

When multiple users are using the same connector, there is sometimes a necessity to reply to support tickets. If there is no logging of the reasons for transfer failures, it becomes difficult to provide effective support services.

Solution Proposal

 if (response.isSuccessful()) {
             ...
            monitor.debug("transfer successes");
            } else {
                try {
                    monitor.debug("transfer fails with status code: "+response.code() );
                    if (NOT_AUTHORIZED == response.code() || FORBIDDEN == response.code()) {
                        return StreamResult.notAuthorized();
                    } else if (NOT_FOUND == response.code()) {
                        return StreamResult.notFound();
                    } else {
                        return error(format("Received code transferring HTTP data: %s - %s.", response.code(), response.message()));
                    }
                   ...

[github-actions]

Thanks for your contribution 🔥 We will take a look asap 🚀

[jimmarino]

Stacktraces and message payloads for requests against internal systems should not be forwarded for security reasons. This can be handled by logging the request failure at the backend and correlating it.

[zub4t]

@jimmarino Is it not possible to record the request failure in the EDC without forwarding the response to the caller? If we can achieve this, it would enhance troubleshooting, especially in scenarios where access to the backend system logs is unavailable.

For example, could the message format("Received code transferring HTTP data: %s - %s.", response.code(), response.message()); also be logged using the monitor?

[jimmarino]

No, we explicitly do not want to do that due to concerns about logging potentially private data.

[gerbigf]

Hey @jimmarino - fully understood. Could we at least add a log statement that tells us if the request to the backend was (un-)successful and with which status code? I know that the partner receives this information, but it helps us to trace the error down ourselves and have a complete log trace. Neither do we as the EDC Ops team or our users always have access to the logs of the backend API, nor does our partner always provide us with the full response.

Knowing if it was e.g. a 404 or 403 already would help, even without getting the complete error message.

[jimmarino]

That should already be done so it should allow you to track it down. If not logging the code is fine

…

On Wed, 19 Jun 2024 at 2:16 PM, Felix Gerbig ***@***.***> wrote: Hey @jimmarino <https://github.com/jimmarino> - fully understood. Could we at least add a log statement that tells us if the request to the backend was (un-)successful and with which status code? I know that the partner receives this information, but it helps us to trace the error down ourselves and have a complete log trace. Neither do we as the EDC Ops team or our users always have access to the logs of the backend API, nor does our partner always provide us with the full response. Knowing if it was e.g. a 404 or 403 already would help, even without getting the complete error message. — Reply to this email directly, view it on GitHub <#4280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4H4IJMSLWXXYPJ5V53C3ZIFZCPAVCNFSM6AAAAABJP5PCOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZYGU2TGNRUGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[gerbigf]

I'm not sure if that works as intended - in our scenario neither there's a log message of any type, nor a proper response message to the partner EDC We're working on reproducing the issue and will open a separate issue if we can reproduce.

[jimmarino]

Ok

…

On Wed, 19 Jun 2024 at 2:59 PM, Felix Gerbig ***@***.***> wrote: I'm not sure if that works as intended - in our scenario neither there's a log message of any type, nor a proper response message to the partner EDC We're working on reproducing the issue and will open a separate issue if we can reproduce. — Reply to this email directly, view it on GitHub <#4280 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4H4OVMB77W3GMZUDFBXLZIF6CXAVCNFSM6AAAAABJP5PCOCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZYGY3DEOBYHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[ndr-brt]

the error detail with the status code should be stored in the DataFlow object when it transition to failed.
The problem here is that it stores the failure.getFailureDetail(), that could be empty if there's no message in the failure.
That seems to be the case according to the code posted in the issue:

if (NOT_AUTHORIZED == response.code() || FORBIDDEN == response.code()) {
                        return StreamResult.notAuthorized();
                    } else if (NOT_FOUND == response.code()) {
                        return StreamResult.notFound();
                    } else {

I see different solutions:

• avoid "empty message" failures
• make the transitionToFailed accept a Failure as argument and put as errorDetail a string representation of the Failure, that, for the StreamFailure case will contain the reason as well
• make the DataPlaneManager to appent the reason to the message that will be stored in the database.

I'd say that the second one is my favurite: passing the Failure object around instead of having String will make the code more expressive (and less primitive-obsessed).

[jimmarino]

the error detail with the status code should be stored in the DataFlow object when it transition to failed. The problem here is that it stores the failure.getFailureDetail(), that could be empty if there's no message in the failure. That seems to be the case according to the code posted in the issue:

if (NOT_AUTHORIZED == response.code() || FORBIDDEN == response.code()) {
                        return StreamResult.notAuthorized();
                    } else if (NOT_FOUND == response.code()) {
                        return StreamResult.notFound();
                    } else {

I see different solutions:

• avoid "empty message" failures
• make the transitionToFailed accept a Failure as argument and put as errorDetail a string representation of the Failure, that, for the StreamFailure case will contain the reason as well
• make the DataPlaneManager to appent the reason to the message that will be stored in the database.

I'd say that the second one is my favurite: passing the Failure object around instead of having String will make the code more expressive (and less primitive-obsessed).

I agree the second option is better. We just need to be careful we do not log the message body or propagate it back to the client.

[zub4t]

Good 😊
@jimmarino or @ndr-brt can you assign this issue to me?