feat(metrics): initialise MULTIPLEXED_METRIC_ROUTING_KEY for routing #19096
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| attributes: { | ||
| ...serializeAttributes(scopeAttributes), | ||
| ...serializeAttributes(metric.attributes, 'skip-undefined'), | ||
| ...serializeAttributes(_stripRoutingAttributes(metric.attributes), 'skip-undefined'), |
There was a problem hiding this comment.
Bug: The _buildSerializedMetric function strips routing attributes from metrics, preventing the multiplexed transport from correctly routing them. The public captureMetric API for routing is non-functional.
Severity: HIGH
Suggested Fix
The _buildSerializedMetric function should not strip the routing attributes. The routing information needs to be preserved in the metric attributes until it reaches the transport layer so that the multiplexed transport can correctly route the metric based on the provided options. The call to _stripRoutingAttributes() within _buildSerializedMetric() should be removed or modified.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/core/src/metrics/internal.ts#L168
Potential issue: When a user calls `captureMetric()` with routing options, the routing
information is added to the metric's attributes. However, during serialization in
`_buildSerializedMetric()`, the `_stripRoutingAttributes()` function is called, which
removes this routing information. Consequently, when the multiplexed transport later
attempts to access the routing key, it is no longer present. This results in all metrics
being sent to the default DSN, rendering the metric routing feature non-functional when
used via the public API. Existing tests pass because they bypass the public API and
serialization flow.
Did we get this right? 👍 / 👎 to inform future reviews.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| attributes: { | ||
| ...serializeAttributes(scopeAttributes), | ||
| ...serializeAttributes(metric.attributes, 'skip-undefined'), | ||
| ...serializeAttributes(_stripRoutingAttributes(metric.attributes), 'skip-undefined'), |
There was a problem hiding this comment.
Metric routing stripped before transport can read it
High Severity
The routing information added via MULTIPLEXED_METRIC_ROUTING_KEY in public-api.ts is stripped by _stripRoutingAttributes() in _buildSerializedMetric() before the metric is added to the buffer. When the multiplexed transport later reads the metric from the envelope, the routing key has already been removed, so metric.attributes[MULTIPLEXED_METRIC_ROUTING_KEY] is undefined. This makes the entire metric routing feature non-functional when using the public API (metrics.count(), metrics.gauge(), metrics.distribution() with routing option).
Additional Locations (1)
There was a problem hiding this comment.
looking into it. Thanks !
| const envelope = createMetricEnvelope([metricWithRouting], undefined, undefined, undefined); | ||
| const transport = makeTransport({ url: DSN1_URL, ...transportOptions }); | ||
| await transport.send(envelope); | ||
| }); |
There was a problem hiding this comment.
Missing integration test for metric routing feature
Low Severity
Per the review rules for feat PRs: "check if the PR includes at least one integration or E2E test." This PR adds only unit tests that test components in isolation. The multiplexed transport tests manually create SerializedMetric objects with routing already present in attributes, and the internal tests verify routing is stripped from the buffer. No test exercises the full flow from metrics.count() with routing option through to the multiplexed transport, which would have caught the logic bug where routing is stripped before the transport can read it.
| metric.attributes = {}; | ||
| } | ||
| metric.attributes['sentry.release'] = { type: 'string', value: release }; | ||
| } |
There was a problem hiding this comment.
Release override only applies to first metric in batch
Medium Severity
The makeOverrideReleaseTransport function is documented to override "the release on all events and metrics" but metricFromEnvelope only returns containerItems[0] - the first metric. When the metrics buffer is flushed, multiple metrics are batched into a single envelope. Only the first metric receives the sentry.release attribute; the remaining metrics in the batch are skipped. This causes inconsistent release tracking data where some metrics have the attribute and others don't.
Additional Locations (1)
1 participant
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #18913
What changed
metricsMetricRoutingInfowith dsn and release as its fieldMetricOptionsand enabledcaptureMetricto inject routing info into attributesInternalCaptureMetricOptions_stripRoutingAttributesfunction which removes routing info before sending to sentry