// AI GENERATED: DO NOT MERGE, PURELY TO REVIEW CODE QUALITY- new nats integration #7120
Conversation
tlhunter
added
AI Generated
BenchmarksBenchmark execution time: 2025-12-16 20:47:29 Comparing candidate commit 39b14b5 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 295 metrics, 25 unstable metrics. |
| "DD_CIVISIBILITY_GIT_UPLOAD_ENABLED": ["A"], | ||
| "DD_CIVISIBILITY_IMPACTED_TESTS_DETECTION_ENABLED": ["A"], | ||
| "DD_CIVISIBILITY_ITR_ENABLED": ["A"], | ||
| "DD_CIVISIBILITY_RUM_FLUSH_WAIT_MILLIS": ["A"], |
| @@ -0,0 +1,279 @@ | |||
| 'use strict' | |||
|
|
|||
| // ⚠️ MUST be set BEFORE any requires that initialize the tracer! | |||
There was a problem hiding this comment.
Might want to add "no emoji" to the prompt lol
| // Register hooks for orchestrion (publish and processMsg) | ||
| // NOTE: request() instrumentation is NOT supported due to NATS internal architecture. | ||
| // Any wrapping of request() breaks the internal inbox/reply correlation mechanism. | ||
| // This is a known limitation documented in the integration. |
There was a problem hiding this comment.
Is this referencing the three lines above it?
There was a problem hiding this comment.
This is really referencing this file: packages/datadog-instrumentations/src/helpers/rewriter/instrumentations/nats.json. and the fact that we tried to instrument request but failed for whatever reason. Will update the reviewer agent prompt again to ensure that we cleanup these comments.
| static get id () { return 'nats' } | ||
| static operation = 'processMsg' |
There was a problem hiding this comment.
Should use the same syntax for both these lines
There was a problem hiding this comment.
Good catch! will update the reviewer agent to catch these!
|
|
||
| class NatsConsumerPlugin extends ConsumerPlugin { | ||
| static get id () { return 'nats' } | ||
| static operation = 'processMsg' |
There was a problem hiding this comment.
Should processMsg be consume? I think we have common a convention for queues?
There was a problem hiding this comment.
we dont have any common conventions for operation naming.
| return ctx.currentStore | ||
| } | ||
|
|
||
| end (ctx) { |
There was a problem hiding this comment.
remove event functions if they match parent event functions.
|
|
||
| module.exports = { | ||
| 'tracing:orchestrion:nats:NatsConnectionImpl_publish': natsconnectionimplPublishProducer, | ||
| 'tracing:orchestrion:nats:ProtocolHandler_processMsg': protocolhandlerProcessmsgConsumer |
There was a problem hiding this comment.
This whole approach is a bit bizarre to look at. The function names are very long, and the property names are channels.
It would make sense to include these directly in the producer and consumer files themselves, but I can't easily tell if both are used in both or not. I guess registerOperation does something here to select one of them? If that's the case, these should be directly inside the file that uses them. (I'm not sure, that's a new method to me.)
|
|
||
| class NatsConsumerPlugin extends ConsumerPlugin { | ||
| static get id () { return 'nats' } | ||
| static operation = 'processMsg' |
There was a problem hiding this comment.
Here and throughout this PR, both a static getter and a static member are used. The static getter is a holdover from when static members weren't supported by the runtime. Today they are, so everything should be a static member.
| const options = Extractors[channel]?.(ctx) | ||
| if (!options) return ctx.currentStore | ||
|
|
||
| const msg = ctx.arguments?.[0] |
There was a problem hiding this comment.
In the start event, there's always an arguments property on the context. There's no need to check for its presence. That's true here and throughout the PR.
| } | ||
| return Object.keys(headers).length > 0 ? headers : null | ||
| } catch { | ||
| return null |
There was a problem hiding this comment.
This is a good candidate for logging.
| 'use strict' | ||
|
|
||
| const ConsumerPlugin = require('../../dd-trace/src/plugins/consumer') | ||
| const Extractors = require('./extractors') |
There was a problem hiding this comment.
Things that aren't classes shouldn't have a leading capital letter. This is not the only place in this PR where this happens.
| // Register hooks for orchestrion (publish and processMsg) | ||
| // NOTE: request() instrumentation is NOT supported due to NATS internal architecture. | ||
| // Any wrapping of request() breaks the internal inbox/reply correlation mechanism. | ||
| // This is a known limitation documented in the integration. |
| })() | ||
|
|
||
| // Send a test message to verify responder is working | ||
| await new Promise(resolve => setTimeout(resolve, 200)) |
There was a problem hiding this comment.
No need for the Promise constructor here. Just use node:timers/promises.
Broadly though, since it doesn't look like subprocesses are being used, there's no need to wait or time anything. We should be able to do the appropriate things when they're ready, not after timeouts. Timeouts like this nearly always add flakiness.
| ]).catch(() => { | ||
| // Ignore timeout errors | ||
| }) |
There was a problem hiding this comment.
This catch call is unnecessary, since you're already catching errors on line 52.
| } | ||
|
|
||
| // --- Operations --- | ||
| async natsconnectionimpl_publish () { |
There was a problem hiding this comment.
None of these function names follow our coding style.
| "DD_TRACE_MYSQL_ENABLED": ["A"], | ||
| "DD_TRACE_MYSQL2_ENABLED": ["A"], | ||
| "DD_TRACE_NATIVE_SPAN_EVENTS": ["A"], | ||
| "DD_TRACE_NATS_ENABLED": ["A"], |
There was a problem hiding this comment.
This appears to be the only change in this file that has anything to do with this PR.
There was a problem hiding this comment.
explained over slack
What does this PR do?
// AI GENERATED PR
this pr is purely to discuss the quality of ai written integrations. please give any reviews, comments, etc. also feel free to reach out to @wconti27 with review comments.
Motivation
Plugin Checklist
Additional Notes