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

Improve clarity of instrumentation specs #2006

Closed
jsumners-nr opened this issue Feb 8, 2024 · 1 comment
Closed

Improve clarity of instrumentation specs #2006

jsumners-nr opened this issue Feb 8, 2024 · 1 comment

Comments

@jsumners-nr
Copy link
Contributor

Description

Our agent has an instrumentation facilitator concept we call "shims." These shims accept a DSL of varying types that describe how a thing should be instrumented. The shim will interpret the DSL and do the appropriate work to create the instrumentation for the thing being instrumented. We call these DSLs "specs."

The current state is that all of these specs are object literals scattered throughout the codebase. As an example, here is a spec in our OpenAI instrumentation:

return {
name: OPENAI.COMPLETION,
promise: true,
// eslint-disable-next-line max-params
after(_shim, _fn, _name, err, response, segment) {
if (request.stream) {
instrumentStream({ shim, request, response, segment })
} else {
recordChatCompletionMessages({
agent,
segment,
request,
response,
err
})
}
segment.transaction.trace.attributes.addAttribute(DESTINATIONS.TRANS_EVENT, 'llm', true)
}
}

The issue with this is that it requires hard won knowledge to know which spec shape is appropriate and what constitutes a valid object to meet the requirements. Further, none of our known spec types are documented. Thus, engineers new to the codebase, and even those familiar with it, have a difficult time learning what is being done when a spec is used, and an even more difficult time understanding the effect the chosen spec will have. Therefore, we should refactor the spec definitions into class style objects and document them as thoroughly as we can.

Acceptance Criteria

  1. Refactor all known specs into class style objects.
  2. Update known usages of the current traditional function style specs into the refactored class style. See
    function WrapSpec(spec) {
    this.wrapper = typeof spec === 'function' ? spec : spec.wrapper
    this.matchArity = hasOwnProperty(spec, 'matchArity') ? spec.matchArity : false
    }
    function SegmentSpec(spec) {
    this.name = hasOwnProperty(spec, 'name') ? spec.name : null
    this.recorder = hasOwnProperty(spec, 'recorder') ? spec.recorder : null
    this.inContext = hasOwnProperty(spec, 'inContext') ? spec.inContext : null
    this.parent = hasOwnProperty(spec, 'parent') ? spec.parent : null
    this.parameters = hasOwnProperty(spec, 'parameters') ? spec.parameters : null
    this.internal = hasOwnProperty(spec, 'internal') ? spec.internal : false
    this.opaque = hasOwnProperty(spec, 'opaque') ? spec.opaque : false
    }
    function RecorderSpec(spec) {
    SegmentSpec.call(this, spec)
    this.stream = hasOwnProperty(spec, 'stream') ? spec.stream : null
    this.promise = hasOwnProperty(spec, 'promise') ? spec.promise : null
    this.callback = hasOwnProperty(spec, 'callback') ? spec.callback : null
    this.rowCallback = hasOwnProperty(spec, 'rowCallback') ? spec.rowCallback : null
    this.after = hasOwnProperty(spec, 'after') ? spec.after : null
    this.callbackRequired = hasOwnProperty(spec, 'callbackRequired') ? spec.callbackRequired : null
    }
    util.inherits(RecorderSpec, SegmentSpec)
    function MiddlewareSpec(spec) {
    RecorderSpec.call(this, spec)
    this.req = hasOwnProperty(spec, 'req') ? spec.req : ARG_INDEXES.FIRST
    this.res = hasOwnProperty(spec, 'res') ? spec.res : ARG_INDEXES.SECOND
    this.next = hasOwnProperty(spec, 'next') ? spec.next : ARG_INDEXES.THIRD
    this.type = hasOwnProperty(spec, 'type') ? spec.type : 'MIDDLEWARE'
    this.route = hasOwnProperty(spec, 'route') ? spec.route : null
    this.params = hasOwnProperty(spec, 'params') ? spec.params : _defaultGetParams
    this.appendPath = hasOwnProperty(spec, 'appendPath') ? spec.appendPath : true
    }
    util.inherits(MiddlewareSpec, RecorderSpec)
    function MessageSpec(spec) {
    RecorderSpec.call(this, spec)
    this.destinationName = hasOwnProperty(spec, 'destinationName') ? spec.destinationName : null
    this.destinationType = hasOwnProperty(spec, 'destinationType') ? spec.destinationType : null
    this.headers = hasOwnProperty(spec, 'headers') ? spec.headers : null
    this.routingKey = hasOwnProperty(spec, 'routingKey') ? spec.routingKey : null
    this.queue = hasOwnProperty(spec, 'queue') ? spec.queue : null
    this.messageHandler = hasOwnProperty(spec, 'messageHandler') ? spec.messageHandler : null
    }
    util.inherits(MessageSpec, RecorderSpec)
  3. Add jsdoc blocks to all new objects and object fields so that engineers have some documentation to consult around specs.
  4. Update at least one instrumentation to utilize the new spec objects as an example for new code going forward.

Design Consideration/Limitations

With the large surface of existing code, it is not feasible to discover and covert all object literal spec instances to the new object style in a single pull request. We will need to update instances as we encounter them in the future, and possible whittle the scope down through subsequent "down time" chores.

@workato-integration
Copy link

@newrelic-node-agent-team newrelic-node-agent-team added this to Triage Needed: Unprioritized Features in Node.js Engineering Board Feb 8, 2024
@jsumners-nr jsumners-nr self-assigned this Feb 8, 2024
@jsumners-nr jsumners-nr moved this from Triage Needed: Unprioritized Features to In progress: Issues being worked on in Node.js Engineering Board Feb 8, 2024
Node.js Engineering Board automation moved this from In progress: Issues being worked on to Done: Issues recently completed Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

No branches or pull requests

1 participant