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.

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

feat: allow manual instrumentation when instrument is set to false #1114

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

Qard
Copy link
Contributor

@Qard Qard commented Jun 5, 2019

Fixes #858

Checklist

  • Implement code
  • Add tests
  • Update documentation

@Qard Qard requested a review from watson June 5, 2019 02:01
@Qard Qard self-assigned this Jun 5, 2019
@Qard Qard force-pushed the instrument-false-manual-instrumentation branch 3 times, most recently from db941e4 to 34130a2 Compare June 7, 2019 21:33
lib/instrumentation/index.js Outdated Show resolved Hide resolved
@watson
Copy link
Contributor

watson commented Jun 11, 2019

We should probably put this in v3.0.0 as it could be considered a breaking change 🤔

@Qard Qard force-pushed the instrument-false-manual-instrumentation branch 2 times, most recently from 8599942 to 9667564 Compare June 11, 2019 23:18
@Qard
Copy link
Contributor Author

Qard commented Jun 11, 2019

True. Marking as breaking change.

@Qard Qard force-pushed the instrument-false-manual-instrumentation branch 2 times, most recently from e728708 to 2a82b8b Compare July 29, 2019 09:30
@watson watson added this to the v3.0.0 milestone Aug 6, 2019
@Qard Qard force-pushed the instrument-false-manual-instrumentation branch 2 times, most recently from d82d594 to ac0be47 Compare September 19, 2019 20:34
@watson
Copy link
Contributor

watson commented Sep 20, 2019

Jenkins run the tests please

watson
watson previously approved these changes Sep 20, 2019
@watson
Copy link
Contributor

watson commented Sep 20, 2019

I'm able to reproduce the Travis failure locally. The issue is that the CPU just starts spinning forever during the test/config.js test. It's happening on the latest Node.js v10, but not v12 when running nyc node test/config.js, but doesn't happen when running node test/config.js. So something that nyc is doing is causing something.

When it occurs, the process hangs so badly that you need to run a kill -9 to exit it.

@watson
Copy link
Contributor

watson commented Sep 23, 2019

It's now Monday, and I upgraded to Safari 13 over the weekend, which required me to agree on a new XCode License Agreement before git would work in the terminal again. And I'm no longer able to reproduce the error locally

image

@watson
Copy link
Contributor

watson commented Sep 23, 2019

Ok, I can reproduce it again - but not every time. Turns out the error doesn't always happen, so sometimes you need to run the test command a few times before it occurs. So I usually run it like this:

while true; do ./node_modules/.bin/nyc node test/config.js || break; done

I made a core dump while the script was stuck in the infinite loop and discovered that it was related to async hooks. When disabling async hooks in the instrument: false allows manual instrumentation test, the error disappeared. This obviously isn't a solution but might help in determining the cause.

@watson
Copy link
Contributor

watson commented Sep 23, 2019

I think I narrowed it down to multiple async hooks being enabled during the test/config.js tests. If I put the return value of asyncHooks.createHook() into a global variable on which I call .disable() if it's already present when trying to create a new hook, then the problem goes away.

Working on a fix...

@watson
Copy link
Contributor

watson commented Sep 23, 2019

I've fixed the CI issue in #1373. Once that's merged, master needs to be merged into this PR and it should hopefully be ready to merge

@Qard Qard force-pushed the instrument-false-manual-instrumentation branch from 9efebeb to 582d18a Compare September 23, 2019 20:41
@watson
Copy link
Contributor

watson commented Sep 24, 2019

I think you accidentally force pushed a version that removed my last commit. I assume that was just because you rebased with master without pulling first?

I'll force push a new version with the extra commits. Hope you don't mind 🙂

@watson watson force-pushed the instrument-false-manual-instrumentation branch from 582d18a to 3b5e043 Compare September 24, 2019 06:50
@watson
Copy link
Contributor

watson commented Sep 24, 2019

jenkins run the tests please

@Qard
Copy link
Contributor Author

Qard commented Sep 24, 2019

Ah, sorry about that. I missed your commits. 😅

@Qard
Copy link
Contributor Author

Qard commented Sep 24, 2019

jenkins run the tests please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instrument: false should disable automatic instrumentation
2 participants