Improve Workers example - #14
Conversation
b5c7f07 to
6e458fc
Compare
6e458fc to
06e94ec
Compare
bahlo
left a comment
bahlo
left a comment
There was a problem hiding this comment.
First of all, thank you so much for the review and sorry for taking so long to review!
This looks great in general, would be even better as an npm package (as you wrote as well), but we can do that in a follow-up.
The biggest thing is that this takes the worker from logging requests to providing a logger implementation and, while the logger is a great addition, the use-case of logging requests and being able to inspect req/res/metadata is just as valid.
Can we add that functionality back please?
| "description": "send logs form cloudflare to axiom", | ||
| "main": "src/worker.js", | ||
| "author": "Axiom, Inc.", | ||
| "name": "workers-axiom-example", |
There was a problem hiding this comment.
Could we keep the old metadata like name, contributors, repository, keywords, etc.?
| /** Version of the Workers application */ | ||
| version?: string | ||
| /** Data included in every log */ | ||
| data?: any |
There was a problem hiding this comment.
| data?: any | |
| data?: { [key: string]: any } |
| data?: any | ||
| /** ID unique to each invocation. Default: crypto.randomUUID() */ | ||
| invocationId?: string | ||
| /** Axiom ingest API endpoint. Default: https://api.axiom.co */ |
| /** Flush after this many ms. Default: 10,000 */ | ||
| flushAfterMs?: number | ||
| /** Flush after this many logs. Default: 100 */ | ||
| flushAfterLogs?: number |
There was a problem hiding this comment.
Please default to 1s/1000 logs for consistent behaviour with other Axiom tools
| this.logs.splice(0, logsCount) | ||
| await res.arrayBuffer() // Read the body to completion | ||
| } else { | ||
| console.log(`axiom failed to ingest logs: ${res.status} ${res.statusText} ${await res.text()}`) |
There was a problem hiding this comment.
| console.log(`axiom failed to ingest logs: ${res.status} ${res.statusText} ${await res.text()}`) | |
| console.warn(`Axiom failed to ingest logs: ${res.status} ${res.statusText} ${await res.text()}`) |
| console.log(`axiom failed to ingest logs: ${res.status} ${res.statusText} ${await res.text()}`) | ||
| } | ||
| } catch (err) { | ||
| console.error(`axiom failed to ingest logs: ${err}`) |
There was a problem hiding this comment.
| console.error(`axiom failed to ingest logs: ${err}`) | |
| console.error(`Axiom failed to ingest logs: ${err}`) |
| // Make sure the last one is done before starting a flush | ||
| await this.flushPromise | ||
|
|
||
| this.flushPromise = doFlush() |
There was a problem hiding this comment.
If you call flush multiple times, won't they all await this.flushPromise and, once done, race to set this.flushPromise?
2 participants
Been using something similar to this for my Workers for a few months and have ingested ~38M logs so far from Workers, so thought I'd make a PR.
This improves this example with a few improvements:
npm create cloudflare@latest"Hello World worker" templateThis logger class can be used with all handler types (fetch/queues/email/etc.) I removed some of the request-based example we had before so that it's easier for users to use with non-http handlers. In a future PR, I could move this to a monorepo so that it's easier to create additional examples
Testing
Deployed to Workers and verified logs show up in my Axiom account as expected