Skip to content

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Dec 18, 2025

This removes a test that only asserted on successful deployment of a static cloudflare app. No actual tests. I don't think there's much value in this but it required a repo-specific env secret we can now get rid of.

(fwiw, long-term we definitely want cloudflare-deployed e2e tests but we'll likely need a better test setup anyway)

Closes #18568 (added automatically)

}
}

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The function metricAttributeToSerializedMetricAttribute is removed but is still called within _buildSerializedMetric, which will cause a ReferenceError when metrics are captured.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The function metricAttributeToSerializedMetricAttribute is removed in this pull request, but a call to it remains within the _buildSerializedMetric function in the same file. When _INTERNAL_captureMetric() is called, it triggers _buildSerializedMetric, which then attempts to execute the non-existent function. This will lead to a ReferenceError: metricAttributeToSerializedMetricAttribute is not defined, causing a crash whenever metrics are captured. Additionally, the test file packages/core/test/lib/metrics/internal.test.ts still attempts to import and test this deleted function.

💡 Suggested Fix

Either restore the metricAttributeToSerializedMetricAttribute function if its removal was unintentional, or remove the call to it from the _buildSerializedMetric function and update the logic accordingly. The corresponding import and tests for the deleted function in packages/core/test/lib/metrics/internal.test.ts must also be removed.

🤖 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#L17

Potential issue: The function `metricAttributeToSerializedMetricAttribute` is removed in
this pull request, but a call to it remains within the `_buildSerializedMetric` function
in the same file. When `_INTERNAL_captureMetric()` is called, it triggers
`_buildSerializedMetric`, which then attempts to execute the non-existent function. This
will lead to a `ReferenceError: metricAttributeToSerializedMetricAttribute is not
defined`, causing a crash whenever metrics are captured. Additionally, the test file
`packages/core/test/lib/metrics/internal.test.ts` still attempts to import and test this
deleted function.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7712917


const MAX_METRIC_BUFFER_SIZE = 1000;

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Production function deleted but still referenced elsewhere

The PR claims to only remove the cloudflare-astro e2e test, but it also deletes the metricAttributeToSerializedMetricAttribute function from production code. This function is still called in _buildSerializedMetric (line 132), which will cause a build failure. This appears to be an unintended change that was accidentally included with the e2e test removal.

Fix in Cursor Fix in Web

@Lms24 Lms24 self-assigned this Dec 18, 2025
@Lms24 Lms24 requested a review from cleptric December 18, 2025 16:16
@github-actions
Copy link
Contributor

node-overhead report 🧳

Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 8,882 - 11,630 -24%
GET With Sentry 1,769 20% 2,068 -14%
GET With Sentry (error only) 6,127 69% 7,869 -22%
POST Baseline 1,195 - 1,212 -1%
POST With Sentry 617 52% 622 -1%
POST With Sentry (error only) 1,060 89% 1,092 -3%
MYSQL Baseline 3,318 - 3,985 -17%
MYSQL With Sentry 455 14% 614 -26%
MYSQL With Sentry (error only) 2,719 82% 3,219 -16%

View base workflow run

@Lms24 Lms24 merged commit d1dd308 into develop Dec 18, 2025
111 of 114 checks passed
@Lms24 Lms24 deleted the lms/chore-remove-cloudflare-astro-test branch December 18, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore(test): Remove cloudflare-astro e2e test

3 participants