Skip to content

Conversation

jlvoiseux
Copy link
Contributor

@jlvoiseux jlvoiseux commented Apr 14, 2022

Summary

This pull request propagates the ELASTIC_APM_DATA_FORWARDER_TIMEOUT_SECONDS parameter to the configuration of the reverse proxy tasked with serving APM Server info requests. A error handler is also defined to enable backoff upon transaction failure. Associated tests have also been written in main_test.

With this PR, backoff should be enabled after all failed attempts to communicate with the APM Server.

Changes

  • Propagate ctx and config through handleInfoRequest
  • Set the ResponseHeaderTimeout of the reverse proxy to ELASTIC_APM_DATA_FORWARDER_TIMEOUT_SECONDS
  • Define an error handler triggered upon transport failure
  • Modify main_test accordingly

How to test

  • Set up a slow / unresponsive APM Server
  • Setup a basic Python APM-instrumented Lambda function
  • Set an large timeout (30+ seconds)
  • Upon cold start, the Lambda function execution should last between 3 to 5 seconds, and an error containing Error querying version from the APM Server should be logged.

@github-actions github-actions bot added the aws-λ-extension AWS Lambda Extension label Apr 14, 2022
@ghost
Copy link

ghost commented Apr 14, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-04-25T15:24:25.018+0000

  • Duration: 7 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 192
Skipped 4
Total 196

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@jlvoiseux jlvoiseux changed the title Manage info request Make info request timeout configurable Apr 14, 2022
@jlvoiseux jlvoiseux marked this pull request as ready for review April 14, 2022 10:43
Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Apart from the comment about the transport, PR LGTM.

reverseProxy := httputil.NewSingleHostReverseProxy(parsedApmServerUrl)

reverseProxyTimeout := time.Duration(config.DataForwarderTimeoutSeconds) * time.Second
reverseProxy.Transport = http.DefaultTransport
Copy link
Contributor

@simitt simitt Apr 26, 2022

Choose a reason for hiding this comment

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

The caller of httputil.NewSingleHostReverseProxy(parsedApmServerUrl) should not need to have any knowledge about internas of how the reverseProxy is built. With this change the whole transport of the reverseProxy is still switched out; if any other settings (TLS, further timeouts etc) were set in the proxy they would be lost. What I originally meant, was that only the timeout should be customized here, or even better, be passed into the abstracting httputil.NewSingleHostReverseProxy method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-λ-extension AWS Lambda Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants