fix: log plugin name in case the handler throws (#6947)

In case a plugin handler fails, it will deactivate the plugin.
While doing so, the plugin that failed is logged. The id is placed
on the constructor though, not on the instance. Due to that, it
would log undefined so far.
Fixing it by using the constructor is also not ideal though, due
to having more than a single plugin with the same id. That id
represents the instrumentation, not the plugin itself.
An instrumentation can actually have multiple plugins and the
failure will only deactivate a specific plugin. Therefore, the
information about what plugin will be deactivated should log the
name of the plugin instead of the id.

Author: BridgeAR

packages/dd-trace/src/plugins/plugin.js

@@ -118,18 +118,13 @@ module.exports = class Plugin {
    * @returns {void}
    */
   addSub (channelName, handler) {
-    const plugin = this
-    /**
-     * @this {unknown}
-     * @returns {unknown}
-     */
-    const wrappedHandler = function () {
+    const wrappedHandler = (...args) => {
       try {
-        return handler.apply(this, arguments)
-      } catch (e) {
-        logger.error('Error in plugin handler:', e)
-        logger.info('Disabling plugin: %s', plugin.id)
-        plugin.configure(false)
+        return handler.apply(this, args)
+      } catch (error) {
+        logger.error('Error in plugin handler:', error)
+        logger.info('Disabling plugin: %s', this.constructor.name)
+        this.configure(false)
       }
     }
     this._subscriptions.push(new Subscription(channelName, wrappedHandler))

packages/dd-trace/test/plugins/plugin.spec.js

@@ -1,12 +1,22 @@
 'use strict'
 
-const { expect } = require('chai')
+const assert = require('node:assert')
+
 const { describe, it, after } = require('tap').mocha
 const { channel } = require('dc-polyfill')
+const proxyquire = require('proxyquire')
+const sinon = require('sinon')
 
 require('../setup/core')
 
-const Plugin = require('../../src/plugins/plugin')
+const log = {
+  error: sinon.stub(),
+  info: sinon.stub()
+}
+
+const Plugin = proxyquire('../../src/plugins/plugin', {
+  '../log': log
+})
 const { storage } = require('../../../datadog-core')
 
 describe('Plugin', () => {
@@ -34,7 +44,7 @@ describe('Plugin', () => {
     }
 
     start () {
-      //
+      assert.strictEqual(this, plugin)
     }
   }
 
@@ -46,22 +56,25 @@ describe('Plugin', () => {
     plugin = new BadPlugin()
     plugin.configure({ enabled: true })
 
-    expect(plugin._enabled).to.be.true
+    assert.strictEqual(plugin._enabled, true)
 
     channel('apm:badPlugin:start').publish({ foo: 'bar' })
 
-    expect(plugin._enabled).to.be.false
+    sinon.assert.calledWith(log.error, 'Error in plugin handler:', sinon.match.instanceOf(Error))
+    sinon.assert.calledWith(log.info, 'Disabling plugin: %s', 'BadPlugin')
+
+    assert.strictEqual(plugin._enabled, false)
   })
 
   it('should not disable with no error', () => {
     plugin = new GoodPlugin()
     plugin.configure({ enabled: true })
 
-    expect(plugin._enabled).to.be.true
+    assert.strictEqual(plugin._enabled, true)
 
     channel('apm:goodPlugin:start').publish({ foo: 'bar' })
 
-    expect(plugin._enabled).to.be.true
+    assert.strictEqual(plugin._enabled, true)
   })
 
   it('should run binding transforms with an undefined current store', () => {
@@ -79,7 +92,7 @@ describe('Plugin', () => {
 
     storage('legacy').run({ noop: true }, () => {
       channel('apm:test:start').runStores({ currentStore: undefined }, () => {
-        expect(storage('legacy').getStore()).to.equal(undefined)
+        assert.strictEqual(storage('legacy').getStore(), undefined)
       })
     })
   })