-
Notifications
You must be signed in to change notification settings - Fork 422
chore: Refactored zlib instrumentation to be subscriber based
#4233
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| module.exports = { | ||
| zlib: [ | ||
| { | ||
| path: './core/zlib/tc.js', | ||
| instrumentations: [] | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const zlib = require('node:zlib') | ||
| // eslint-disable-next-line n/no-unsupported-features/node-builtins | ||
| const { tracingChannel } = require('node:diagnostics_channel') | ||
|
|
||
| const { | ||
| TracingChannelSubscription, | ||
| TracingChannelSubscriber | ||
| } = require('#agentlib/subscribers/tracing-channel-subscriber.js') | ||
| const { wrapMethod } = require('#agentlib/subscribers/wrap-method.js') | ||
|
|
||
| const methods = [ | ||
| 'deflate', | ||
| 'deflateRaw', | ||
| 'gzip', | ||
| 'gunzip', | ||
| 'inflate', | ||
| 'inflateRaw', | ||
| 'unzip' | ||
| ] | ||
| const subscriptions = [] | ||
| for (const method of methods) { | ||
| // First we need to monkey-patch the module methods to generating | ||
| // tracing channel events. | ||
| const chanName = `nr_core:zlib:${method}` | ||
| wrapMethod({ | ||
| module: zlib, | ||
| methodName: method, | ||
| wrapper(originalMethod, methodName) { | ||
| const chan = tracingChannel(chanName) | ||
| const data = { segmentName: `zlib.${methodName}` } | ||
| return function wrappedMethod(...args) { | ||
| chan.traceCallback(originalMethod, -1, data, this, ...args) | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| // Now we can create a subscription for the channel we created. | ||
| const sub = new TracingChannelSubscription({ channel: chanName }) | ||
| subscriptions.push(sub) | ||
| } | ||
|
|
||
| class ZlibSubscriber extends TracingChannelSubscriber { | ||
| constructor({ agent, logger }) { | ||
| super({ agent, logger, packageName: 'zlib', subs: subscriptions }) | ||
| } | ||
| } | ||
|
|
||
| module.exports = ZlibSubscriber |
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if we really need this. This comes from the work I was experimenting with around |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| /** | ||
| * This is an interface class. It defines the methods each subclass _must_ | ||
| * implement and override in order for the diagnostics channel or tracing | ||
| * channel subscriptions to work. | ||
| * | ||
| * @property {object} agent A New Relic Node.js agent instance. | ||
| * @property {object} logger An agent logger instance. | ||
| * @property {object} config The agent configuration object. | ||
| * @property {string} packageName The name of the module being instrumented. | ||
| * This is the same string one would pass to the `require` function. | ||
| * @property {string} id An alias for `packageName`. | ||
| * | ||
| * @private | ||
| * @interface | ||
| */ | ||
| class Subscriber { | ||
| #agent | ||
| #config | ||
| #logger | ||
| #packageName | ||
|
|
||
| constructor({ agent, logger, packageName }) { | ||
| this.#agent = agent | ||
| this.#config = agent.config | ||
| this.#logger = logger.child({ component: `${packageName}-subscriber` }) | ||
| this.#packageName = packageName | ||
| } | ||
|
|
||
| get [Symbol.toStringTag]() { | ||
| return 'Subscriber' | ||
| } | ||
|
|
||
| get agent() { | ||
| return this.#agent | ||
| } | ||
|
|
||
| get config() { | ||
| return this.#config | ||
| } | ||
|
|
||
| get id() { | ||
| return this.#packageName | ||
| } | ||
|
|
||
| get logger() { | ||
| return this.#logger | ||
| } | ||
|
|
||
| get packageName() { | ||
| return this.#packageName | ||
| } | ||
|
|
||
| /** | ||
| * Indicates if the subscription should be enabled or not. The most likely | ||
| * scenario is that an implementation will consult the agent configuration | ||
| * to determine the result. | ||
| * | ||
| * @returns {boolean} Subscriber is enabled or not. | ||
| */ | ||
| get enabled() { | ||
| throw Error('enabled is not implemented on class: ' + this.constructor.name) | ||
| } | ||
|
|
||
| /** | ||
| * Implementations should utilize subclass specific configuration or logic | ||
| * to enable the subscriber. This is basically a start-up lifecycle hook | ||
| * that the implementation can use to perform necessary actions, e.g. | ||
| * creating an asynchronous context and binding it to an appropriate channel. | ||
| * | ||
| * @returns {void | Function | boolean} Result of the enablement. Not likely | ||
| * to be used. | ||
| */ | ||
| enable() { | ||
| throw Error('enable is not implemented on class: ' + this.constructor.name) | ||
| } | ||
|
|
||
| /** | ||
| * The inverse of the `enable` method. It's basically an agent shutdown | ||
| * lifecycle hook. Any clean up logic required as a result of the work | ||
| * performed in the `enable` method should be hosted here. | ||
| * | ||
| * @returns {void | boolean} Result of the disablement. Not likely to be | ||
| * used. | ||
| */ | ||
| disable() { | ||
| throw Error('disable is not implemented on class: ' + this.constructor.name) | ||
| } | ||
|
|
||
| /** | ||
| * Classes must implement this method. It is expected to read some | ||
| * configuration data, specific to the subclass, and utilize it to | ||
| * perform the channel subscriptions. | ||
| * | ||
| * @returns {void} | ||
| * | ||
| * @example A basic "Diagnostics Channel" based method. | ||
| * const dc = require('node:diagnostics_channel') | ||
| * for (const sub of this.#subscriptions) { | ||
| * dc.subscribe(sub.channelName, sub.hook.bind(this)) | ||
| * } | ||
| */ | ||
| subscribe() { | ||
| throw Error('subscribe is not implemented on class: ' + this.constructor.name) | ||
| } | ||
|
|
||
| /** | ||
| * The inverse of the `subscribe` method. This should iterate through the | ||
| * subscribed channels and issue any unsubscribe and clean-up logic for them. | ||
| * | ||
| * @returns {void} | ||
| */ | ||
| unsubscribe() { | ||
| throw Error('unsubscribe is not implemented on class: ' + this.constructor.name) | ||
| } | ||
| } | ||
|
|
||
| module.exports = Subscriber |
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.
I get that accessing
_prefixed fields within our code base is acceptable and the actual intention of those fields. But I think, even within our own code base, it is better to expose things via public fields. It gives users confidence that they are doing the right thing instead of the anxiety induced by utilizing conventionally private fields.