Fix #1770 support for proxies in destination context

[ebuildy]

Fix #1770 issue:

If a proxy is used, the destination context will not be set on outgoing http spans because the URL cannot be successfully extracted from the request.

Copy/paste from Qard/http-request-to-url@7406c37

Checklist

• Implement code
• Add tests
• Update TypeScript typings
• Update documentation
• Add CHANGELOG.asciidoc entry
• Commit message follows commit guidelines

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: astorm commented: jenkins run the tests please

• Start Time: 2021-06-29T05:35:11.475+0000

• Duration: 19 min 24 sec

• Commit: 951d000

Test stats 🧪

Test	Results
Failed	0
Passed	19040
Skipped	0
Total	19040

Trends 🧪

[trentm]

@ebuildy Thanks.

The best original discussion of this is here: Qard/http-request-to-url#1
There is also a test case example there. Can we take that and add to the test suite here?

[trentm]

jenkins run the tests

[trentm]

I'm not clear on the history of commits here. Is this proxy functionality something we missed in #2039 where the 'http-request-to-url' dep was removed?

[ebuildy]

There is an old bug in case we use HTTP proxy to send query, the URL from request.path == proxied URL (the URL we want to report in APM). The problem is this throw error all the time (see

apm-agent-nodejs/lib/instrumentation/http-shared.js

Line 224 in a2c4777

agent.logger.error('Could not set destination context: %s', e.message)

) and fill logs (whereas the URL is valid).

[astorm]

jenkins run the tests please

[astorm]

There is an old bug in case we use HTTP proxy to send query, the URL from request.path == proxied URL (the URL we want to report in APM). The problem is this throw error all the time (see

apm-agent-nodejs/lib/instrumentation/http-shared.js

Line 224 in a2c4777

agent.logger.error('Could not set destination context: %s', e.message)

) and fill logs (whereas the URL is valid).

Additional Context: We had hoped/assumed that removing the dependency on http-request-to-url would fix this behavior. However, it did not. FWIW I managed to cobble together a proxy server that reproduced the behavior we're trying to fix (#1770 (comment)) so we should be able to validate the fix this time.

[ebuildy]

Wonderful!

It looks like a bug from nodeJS it-self actually, there is no way to guess if a https://github.com/nodejs/node/blob/master/lib/_http_client.js ClientRequest is using a proxy or not.

[astorm]

@elasticmachine, run elasticsearch-ci/docs

[astorm]

It looks like our CI is happy with things. Per @trentm's comments above

Can we take that and add to the test suite here?

I've written up a test that captures the intent of this fix, based on the http-request-to-url test
https://gist.github.com/astorm/ff0211e076d670d3c41e8fe0169f8532

@ebuildy -- would you be able to

1. Add the test at the above gist to your PR
2. Add an entry to the CHANGELOG.asciidoc mentioning this bug fix?

With those in place we should be able to merge.

[astorm]

jenkins run the tests please

[astorm]

@elasticmachine, run elasticsearch-ci/docs

[astorm]

@ebuildy Apologies -- it looks like there's a timing issue in the test I handed you (only pops up in Node 8.6). I made a PR to your branch to fix that. https://github.com/ebuildy/apm-agent-nodejs/pull/1/files

[ebuildy]

Ok got it! thanks you. (Sorry for the delay, I am in France ....)

[astorm]

@ebuildy No rush intended or asked for :)

Also -- PR to fix the above linting errors: https://github.com/ebuildy/apm-agent-nodejs/pull/2

[ebuildy]

sure! just saying that because I answer always a day after. Thanks you for the fix

[astorm]

jenkins run the tests please

[astorm]

@elasticmachine, run elasticsearch-ci/docs

[astorm]

jenkins run the tests please

[astorm]

@elasticmachine, run elasticsearch-ci/docs

[astorm]

👍 does the thing, approving and merging