-
Notifications
You must be signed in to change notification settings - Fork 422
feat(kafka): add MessageBroker/Kafka/Cluster/{id}/Produce|Consume/{topic} metrics #4044
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
075c9ab
5e0d138
43b2579
3699056
ad861fa
3fa461c
0255bfc
77d8f65
08eea64
7dfb67a
d52141a
8fa0ddc
fc2f52e
30e6ea3
690ca74
720daec
3b84613
d1adeed
f933db5
a4db3b4
008c578
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
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. Why are the changes in this file not part of the exported class? Are the two caches really necessary? What is the expected size of these caches in a typical, or extreme, deployment scenario?
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. Hi @jsumners-nr, I'm new to this repo and leaned heavily on AI assistance to implement this, so I apologize upfront for anything that doesn't follow conventions here. The two caches serve different lifecycles:
In a typical deployment you'd have 1–3 distinct Kafka clusters, so the cache holds 1–3 entries. In an extreme multi-tenant scenario (many distinct broker strings) it could grow unbounded - that's a fair concern. Happy to add a size cap or TTL if you'd prefer. Open to moving the fetch logic inside the class if that's the convention - I'll follow your guidance. Can you please guide me?
Contributor
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. There will only be one instance of If it is possible for the cache to grow infinitely, then a size limited LRU cache should be used. We do not currently include such cache functionality. So if this is really a necessary feature, we will need to implement
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. Addressed in the latest commit ( On LRU vs hard cap: I opted for a simpler If you would prefer a true LRU, I am happy to implement |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const Subscriber = require('../base.js') | ||
| const { kafkaCluster } = require('#agentlib/symbols.js') | ||
|
|
||
| /** | ||
| * Same capture as `producer-cluster-capture.js`, but for the internal | ||
| * consumer factory (`kafkajs/src/consumer/index.js`) — kafkajs also | ||
| * constructs a brand new `Cluster` instance on every call to | ||
| * `Kafka.prototype.consumer()`. | ||
| * | ||
| * @type {ConsumerClusterCaptureSubscriber} | ||
| */ | ||
| module.exports = class ConsumerClusterCaptureSubscriber extends Subscriber { | ||
| constructor({ agent, logger }) { | ||
| super({ agent, logger, channelName: 'nr_consumerClusterCapture', packageName: 'kafkajs' }) | ||
| this.requireActiveTx = false | ||
| this.events = ['end'] | ||
| } | ||
|
|
||
| get enabled() { | ||
| if (this.agent.config.feature_flag.kafkajs_instrumentation === false) { | ||
| return false | ||
| } | ||
|
|
||
| return super.enabled | ||
| } | ||
|
|
||
| end(data, ctx) { | ||
| const cluster = data?.arguments?.[0]?.cluster | ||
| if (cluster && data.result) { | ||
| data.result[kafkaCluster] = cluster | ||
| } | ||
| return ctx | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const Subscriber = require('../base.js') | ||
| const { kafkaCluster } = require('#agentlib/symbols.js') | ||
|
|
||
| /** | ||
| * kafkajs constructs a brand new internal `Cluster` instance on every call | ||
| * to `Kafka.prototype.producer()` — it is not reachable from the top-level | ||
| * `Kafka` client. This subscriber intercepts the internal producer factory | ||
| * (`kafkajs/src/producer/index.js`) at the point where kafkajs itself | ||
| * passes the freshly created `cluster` in, and stashes a reference to it on | ||
| * the returned producer instance so `read-cluster-id.js` can read the | ||
| * cluster id straight off it later — no admin connection, no cache needed. | ||
| * | ||
| * @type {ProducerClusterCaptureSubscriber} | ||
| */ | ||
| module.exports = class ProducerClusterCaptureSubscriber extends Subscriber { | ||
| constructor({ agent, logger }) { | ||
| super({ agent, logger, channelName: 'nr_producerClusterCapture', packageName: 'kafkajs' }) | ||
| this.requireActiveTx = false | ||
| this.events = ['end'] | ||
| } | ||
|
|
||
| get enabled() { | ||
| if (this.agent.config.feature_flag.kafkajs_instrumentation === false) { | ||
| return false | ||
| } | ||
|
|
||
| return super.enabled | ||
| } | ||
|
|
||
| end(data, ctx) { | ||
| const cluster = data?.arguments?.[0]?.cluster | ||
| if (cluster && data.result) { | ||
| data.result[kafkaCluster] = cluster | ||
| } | ||
| return ctx | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| const { kafkaCluster } = require('#agentlib/symbols.js') | ||
|
|
||
| /** | ||
| * Reads the Kafka cluster id off a producer/consumer instance. The `Cluster` | ||
| * reference is captured at producer()/consumer() creation time (see | ||
| * `producer-cluster-capture.js` / `consumer-cluster-capture.js`) and stashed | ||
| * on the instance via the `kafkaCluster` symbol. kafkajs keeps | ||
| * `cluster.brokerPool.metadata` current on its own as part of normal | ||
| * operation, so this is a synchronous, in-memory, best-effort read — no | ||
| * network call, no cache. | ||
| * | ||
| * @param {object} instance Producer or consumer client instance. | ||
| * @returns {string|undefined} The cluster id, or `undefined` if not yet | ||
| * available (e.g. a producer's very first send, before kafkajs has fetched | ||
| * any metadata). | ||
| */ | ||
| module.exports = function readClusterId(instance) { | ||
| try { | ||
| const clusterId = instance?.[kafkaCluster]?.brokerPool?.metadata?.clusterId | ||
| return typeof clusterId === 'string' && clusterId !== '' ? clusterId : undefined | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| * Copyright 2026 New Relic Corporation. All rights reserved. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| 'use strict' | ||
|
|
||
| /** | ||
| * Records MessageBroker/Kafka/Cluster cluster-level produce metrics. | ||
| * For send() records one metric for the single topic. | ||
| * For sendBatch() records one metric per distinct topic in the batch. | ||
| * | ||
| * @param {object} metrics The agent metrics aggregator. | ||
| * @param {string} clusterId Kafka cluster UUID. | ||
| * @param {boolean} batch Whether this is a sendBatch call. | ||
| * @param {object} data The send/sendBatch arguments object. | ||
| */ | ||
| module.exports = function recordClusterProduceMetrics(metrics, clusterId, batch, data) { | ||
| if (batch === false) { | ||
| if (!Array.isArray(data?.messages)) { | ||
| return | ||
| } | ||
| metrics | ||
| .getOrCreateMetric(`MessageBroker/Kafka/Cluster/${clusterId}/Produce/${data.topic}`) | ||
| .incrementCallCount(data.messages.length) | ||
| } else { | ||
| if (!Array.isArray(data?.topicMessages)) { | ||
| return | ||
| } | ||
| for (const topicMessage of data.topicMessages) { | ||
| if (!Array.isArray(topicMessage?.messages)) { | ||
| continue | ||
| } | ||
| metrics | ||
| .getOrCreateMetric( | ||
| `MessageBroker/Kafka/Cluster/${clusterId}/Produce/${topicMessage.topic}` | ||
| ) | ||
| .incrementCallCount(topicMessage.messages.length) | ||
| } | ||
| } | ||
| } |
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.
For future eyes. The spec states
be gated behind an opt-in configurationkafka.metrics.cluster.metrics.enabled(disabled by default). This is a feature flag, not configuration. it should live inlib/config/default.js