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.

Sign up for GitHub

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

Jump to bottom

feat(http): add monitorIncomingHTTPRequests config #1298

Merged
merged 5 commits into from Sep 20, 2019
Merged

feat(http): add monitorIncomingHTTPRequests config #1298

merged 5 commits into from Sep 20, 2019

Conversation

Qard
Copy link
Contributor

@Qard Qard commented Aug 21, 2019 •
edited

PR for splitting disabling of outgoing http requests from incoming http requests.

Closes #434

Checklist

  • Implement code
  • Add tests
  • Update documentation

@Qard Qard requested a review from watson August 21, 2019 17:00
@Qard Qard self-assigned this Aug 21, 2019
@Qard Qard force-pushed the split-http branch 2 times, most recently from 76aaef6 to 8aa4344 Compare August 21, 2019 23:55
@watson
Copy link
Member

watson commented Aug 22, 2019

Tip: You can use the new Draft PR feature of GitHub for WIP PR's 😃

watson
watson reviewed Aug 22, 2019
View reviewed changes
Copy link
Member

@watson watson left a comment

Choose a reason for hiding this comment

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

Looks good so far...

I do think it might be easy to misunderstand what disableInstrumentations: ['http'] does with the introduction of monitorIncomingHTTPRequests. A user could easily think that disableInstrumentations: ['http'] would disable both incoming and outgoing, while only monitorIncomingHTTPRequests: false would disable only incoming. But I guess this is just a matter of proper documentation.

lib/config.js Outdated Show resolved Hide resolved
test/instrumentation/_agent.js Show resolved Hide resolved
test/instrumentation/_agent.js Outdated Show resolved Hide resolved
test/instrumentation/_agent.js Outdated Show resolved Hide resolved
@Qard Qard mentioned this pull request Aug 22, 2019
Merged
2 tasks
@watson watson added this to the v3.0.0 milestone Aug 23, 2019
@Qard Qard force-pushed the split-http branch 2 times, most recently from 8ce0462 to d6d7177 Compare August 27, 2019 22:34
watson
watson reviewed Aug 28, 2019
View reviewed changes
Copy link
Member

@watson watson left a comment

Choose a reason for hiding this comment

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

It all looks good 💯

I think all we're missing now is:

  • Docs
  • Update TypeScript typings for the config options object (AgentConfigOptions)
  • And I'm just wondering if it would be worth it to add a test to see what happens if you set this new config option to false. For example where you then start an HTTP server which, when it gets an incoming HTTP request starts a manual transaction and make an outgoing HTTP request. The make a request to this HTTP server and check that it didn't try to create a transaction automatically but still created the manual transaction and a span for the outgoing request.

@Qard Qard force-pushed the split-http branch 3 times, most recently from f673118 to 866836c Compare August 29, 2019 23:48
@Qard
Copy link
Contributor Author

Qard commented Sep 5, 2019

I've narrowed the 12.0 failure to http.request(...) losing context before emitting the 'response' event, which calls the response handler function, if given. I'm not sure yet exactly where that context loss occurs between the the call and the handler. Possibly some internals in net? 🤔

@Qard
Copy link
Contributor Author

Qard commented Sep 9, 2019

The failure is due to the issue fixed by nodejs/node#27477, which didn't land until 12.2.0. As a result, all 12.x releases before 12.2.0 will actually have a context loss in the response handler of all outgoing requests. Apparently our existing tests just happened to not cover that particular case. 🤔

@Qard Qard mentioned this pull request Sep 11, 2019
Merged
1 task
@Qard Qard force-pushed the split-http branch 3 times, most recently from e993455 to feae7bd Compare September 11, 2019 21:20
@watson
Copy link
Member

watson commented Sep 13, 2019

The PR description still says WIP, but I assume it's ready now?

watson
watson previously approved these changes Sep 13, 2019
View reviewed changes
@Qard
Copy link
Contributor Author

Qard commented Sep 13, 2019

Yep, just forgot to update the description. 👍

@watson watson merged commit 130ef6f into elastic:master Sep 20, 2019
watson added a commit to watson/apm-agent-nodejs that referenced this pull request Sep 20, 2019
@watson
test: fix http disabling test
a659ac8 
The PR elastic#1298 introduced a bug in the tests as it expected the span.type
for outgoing http requests to start with `ext.*`. This was changed to
`external.*` in the PR elastic#1291, but elastic#1298 was just never updated to be up
to date with master before it was merged.
@watson watson mentioned this pull request Sep 20, 2019
Merged
watson added a commit that referenced this pull request Sep 20, 2019
@watson
test: fix http disabling test (#1367)
7c88ca3 
The PR #1298 introduced a bug in the tests as it expected the span.type
for outgoing http requests to start with `ext.*`. This was changed to
`external.*` in the PR #1291, but #1298 was just never updated to be up
to date with master before it was merged.
@Qard Qard deleted the split-http branch September 20, 2019 22:36
@trentm trentm mentioned this pull request Sep 9, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@watson watson watson approved these changes

Assignees

@Qard Qard

Labels
breaking change enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow more fine grained disabling of http module features
2 participants
@Qard @watson