-
Notifications
You must be signed in to change notification settings - Fork 98
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
Plugin API: Loading Improvements #372
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit
@@ -116,16 +116,20 @@ function activate(agent) { | |||
if (!patchSet) { | |||
// Load the plugin object | |||
var plugin = originalModuleLoad(instrumentation.file, module, false); | |||
patchSet = []; | |||
patchSet = {}; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
e1faca6
to
d9e3199
Compare
I realize that the amount of change the plugin loader needs to go through (due to bugginess) doesn't instill confidence. So I have added tests that should hopefully showcase that things work smoothly now, and also to bump coverage up. @GoogleCloudPlatform/node-team PTAL |
ce6acd1
to
737fcd6
Compare
737fcd6
to
a087193
Compare
} | ||
|
||
// Future requires get patched as they get loaded. | ||
return function Module_load(request, parent, isMain) { | ||
var instrumentation = plugins[request]; | ||
|
||
if (instrumentation && | ||
agent.config().excludedHooks.indexOf(request) === -1) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@@ -197,19 +147,6 @@ function activate(agent) { | |||
} | |||
|
|||
function deactivate() { | |||
for (var moduleName in plugins) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
// plugin loader as the trace agent. It accepts the list of plugins that the | ||
// plugin loader reads. | ||
function createFakeAgent(plugins) { | ||
function writeToLog(log, data) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
@matthewloring Thanks for taking a look at this again! |
patch.versions
doesn't exist