Skip to content
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

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

hannahhoward
Copy link
Member

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...

Screenshot 2024-11-01 at 1 35 23 AM

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?

Copy link
Member

@alanshaw alanshaw left a 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"
Copy link
Member

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',
Copy link
Member

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....

Copy link
Member Author

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 :)

Copy link
Member Author

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.

@alanshaw
Copy link
Member

alanshaw commented Nov 1, 2024

Great insights.

Also, what the heck is happening between the carpark fetches?

@alanshaw
Copy link
Member

alanshaw commented Nov 1, 2024

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...).

Base automatically changed from feat/use-updated-blob-fetcher to main November 1, 2024 16:59
@hannahhoward
Copy link
Member Author

@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.

@hannahhoward hannahhoward merged commit 894adfe into main Nov 1, 2024
1 check passed
@hannahhoward hannahhoward deleted the feat/instrument branch November 1, 2024 19:14
fforbeck added a commit that referenced this pull request Nov 6, 2024
### 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.
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.

2 participants