assimbly / runtime

Java & REST API's for creating and running integrations
Apache License 2.0
18 stars 3 forks source link

[Http Retry] Response headers interference #269

Closed joepkalthoff closed 3 weeks ago

joepkalthoff commented 2 months ago

Summary

Retries in the Http component include headers from the previous (re)try.

Testcase

Dovetail version: 4.17.0-SNAPSHOT-dcb7cc8 Instance: next Tenant: Regression Tests

Sending Flow: HttpRetrySocketAsynchronous Flow version: 7 url: https://next.dovetail.world/flowdesigner/66b1efbde56ba4001000012e/7/route

Receiving Flow: HttpRetry500 Flow version: 1 url: https://next.dovetail.world/flowdesigner/66b31c0ee56ba4000e00002f/1/route

Input:

Output: Notice the transactions of the Sending flow and the Receiving flow. Especially the body and headers for the first try and the 2 retries. There are screenshots in the results section below:

Result

Initial Http request

Sending Image

Receiving Image

1st retry

Sending Image

Receiving Image

2nd retry

Sending Image

Receiving Image

After the Http retries

Image

Conclusion

I expect the headers from the 5xx responses to not interfere with the retry messages. Only the breadcrumbId, Content-Type and counter header should be included.

There should be a way to see the response headers somewhere though. They can contain valuable information about why the server responded with a 5xx in the first place. Maybe in the Logs of a flow?

The same is true for the 5xx response body. This is a seperate issue: https://github.com/assimbly/runtime/issues/268.

Original issue on Dovetail Application board: https://github.com/dovetailworld/front-end/issues/4635.

skin27 commented 1 month ago

There is already a mechanism that logs all relevant information. I assume this isn't probably referenced to the correct flow, so that it's not in log of the flow (only in the general log on the server). We will investigate this.

skin27 commented 1 month ago

I investigated this, and largely this is by design:

  1. There is a retry step, but we explicitly filter this out of the transactions as this is not seen on the canvas.
  2. The headers are largely removed, because otherwise this will cause issues when resending
  3. The headers/body etc are not logged, because when a message can be resend 100s of times. When we have big messages or large headers, this can fill the log directory very fast on the server.

As this is resend and not send to the error route, it's logged as a warning:

"HTTP Status: 500 Server Error | Flow: ID_66b1efbde56ba4001000012e | API Endpoint: https://next.dovetail.world/test/inbound_http/regressiontests/HttpRetry500 | Retry in 10000 milliseconds | Attempt 1 "

This gives the following warning:

  1. HTTP Error and status code
  2. Flow ID
  3. The Endpoint is calling
  4. Retry interval
  5. The attempt

This is minimal because when there is a 500, this means there is "an internal server error", which means the error won't give any extra useful information I guess, and then it just retries.

joepkalthoff commented 1 month ago

Even with 5xx error the body and headers can certainly contain useful information: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/500

For example: the response body for Bol.com and Channel Engine contain request id's for 5xx responses.

Ideally you would like to see a full representation of the response message, just like any other exchange.

There are some other problems with the current implementation, i.e. timeouts (asynch & queues) triggering while the retry process is still ongoing.

joepkalthoff commented 3 weeks ago

Tested, seems to be no interference anymore.

Did notice the extra headers we are sending that might cause issues with existing Http component implementations. Will discuss with Norman.