-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor(handler): at opentelemtry instrumentation #125
Conversation
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.
I don't really understand it, but I like the screenshot and it looks like it'll give us some good insights.
I would remove the yarn package manager thing from package.json
unless it's required...
package.json
Outdated
@@ -69,5 +72,6 @@ | |||
"ignore": [ | |||
"*.ts" | |||
] | |||
} | |||
}, | |||
"packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e" |
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.
You're using yarn?
if (env.HONEYCOMB_API_KEY) { | ||
return { | ||
exporter: { | ||
url: 'https://api.honeycomb.io/v1/traces', |
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.
Does it get sent for every request? I'm slightly worried we'll run up a large bill or get rate limited immediately....
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.
note that if honeycomb api key is absent it sends nothing :) which for prod, for the timebeing it is :)
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.
we need to have a discussion about paying for honeycomb in general before we do anything other than turn on for dev testing.
Great insights. Also, what the heck is happening between the carpark fetches? |
Random idea that might lead to a reduction in time, we might want to tweak the batch size - it's set to 16 but I think workers can only make 6 concurrent requests at a time (the public bucket is fronted by a worker for multipart byte range support but also because I figured in the future we might have some sort of UCAN auth to do...). |
0d82aeb
to
1243a3f
Compare
@alanshaw I believe the spans only cover the time it takes to call fetch, and not the amount it takes to consume the body of the request. I suspect that's the time required to actually download the data. |
### Fix for Node.js Built-in Module Resolution in Cloudflare Worker Miniflare Tests #### **Overview** This PR addresses an issue with resolving Node.js built-in modules (`node:buffer`, `node:events`, `node:async_hooks`) in a Cloudflare Worker Test environment. The Worker encountered errors in resolving these built-in modules, which aren’t natively supported in Workers. This issue was introduced when the [Opentelemetry PR was merged](#125), but we didn't see that because the Github Action that runs the `test:miniflare` was not flagging the test failure. #### **Error Description** During testing, the following error appeared: ``` Unable to resolve "dist/worker.mjs" dependency "node:buffer": no matching module rules. If you're trying to import a Node.js built-in module, or an npm package that uses Node.js built-ins, you'll either need to: - Bundle your Worker, configuring your bundler to polyfill Node.js built-ins - Configure your bundler to load Workers-compatible builds by changing the main fields/conditions - Enable the `nodejs_compat` compatibility flag and use the `NodeJsCompatModule` module type - Find an alternative package that doesn't require Node.js built-ins ``` #### **Solution** 1. To resolve this, I've enabled the `nodejs_compat` compatibility flag in the `test/miniflare/freeway.spec.js` file - which allows the Cloudflare Miniflare Worker to utilize Node.js runtime APIs, and set the compatibility date to "2024-09-23", as suggested in their docs: https://developers.cloudflare.com/workers/runtime-apis/nodejs/#get-started 2. Updated the Github Action to halt the execution in case of test failures 3. Updated the wrangler package to clean up some warnings during the integration tests. #### **Note** As of September 23, 2024, enabling `nodejs_compat` applies the same behavior as `nodejs_compat_v2` for compatibility dates set to September 23, 2024 or later, meaning this configuration is fully aligned with the latest runtime support for Node.js compatibility in Workers.
Goals
Gain insight into what's going on with freeway
Implementation
When in doubt, instrument. This adds opentelemtry and a default cloudflare open telemetry implementation, that at minimum tells you how long fetch requests are taking. And it's.... very interesting...
This is a trace of about a 64mb file downloading.
I dunno what to make of all it, except to say... ouch those CARPARK downloads take a WHILE. I wonder if somehow accessing them through public URLs is a lot slower?