-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Hook up datadog logger #25
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/rocicorp/replidraw-do/9UJZaujSgPYhosgnYW1mPFaXFUkg |
src/pages/d/[id].tsx
Outdated
@@ -33,7 +33,7 @@ export default function Home() { | |||
|
|||
const workerHost = | |||
process.env.NEXT_PUBLIC_WORKER_HOST ?? | |||
"wss://reps.replicache.workers.dev"; | |||
"wss://replidraw-do.replicache.workers.dev"; |
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.
@aboodman Please confirm that this is correct
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.
Current prod is using reps
but I agree that doesn't make sense. I'm going to change it to just replidraw
since replidraw-do
is superfluous.
constructor(state: DurableObjectState, env: TODO) { | ||
const abortController = new AbortController(); | ||
const logger = new DatadogLogger({ | ||
apiKey: env.DATADOG_API_KEY, |
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.
This needs to come from env :'(
Converting to draft since I cannot get the CF secrets to work... Will try again tomorrow |
src/pages/d/[id].tsx
Outdated
@@ -33,7 +33,7 @@ export default function Home() { | |||
|
|||
const workerHost = | |||
process.env.NEXT_PUBLIC_WORKER_HOST ?? | |||
"wss://reps.replicache.workers.dev"; | |||
"wss://replidraw-do.replicache.workers.dev"; |
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.
Current prod is using reps
but I agree that doesn't make sense. I'm going to change it to just replidraw
since replidraw-do
is superfluous.
@@ -36,3 +36,5 @@ yarn-error.log* | |||
$ CloudFlare worker | |||
.mf/ | |||
tsconfig.tsbuildinfo | |||
.env |
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 must define the env var in here for miniflare to work. See: https://miniflare.dev/core/variables-secrets
worker/index.ts
Outdated
@@ -19,10 +19,6 @@ export class Server extends BaseServer<M> { | |||
mutators, | |||
state, | |||
logger, | |||
async onClose() { |
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.
onClose
gets fired when each connection gets torn down, not when the entire worker gets closed down. Thus it's not correct to abort the logger at this time -- it stops the logger from working after the first client disconnects.
I'm not sure if there is a way to know when the worker is getting shut down.
The relevant code in reps-do should also be removed.
@@ -1,7 +1,7 @@ | |||
name = "replidraw-do" | |||
name = "replidraw" |
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'm moving to this name.
type = "javascript" | ||
|
||
account_id = "84dd7a44a4da314095315fb89b24251c" | ||
account_id = "" |
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 can leave this empty and login via wrangler instead. This makes it a bit easier for customers to reuse.
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 think that is correct:
👀 You have multiple accounts.
🕵️ You can copy your account_id below
+--------------------------+----------------------------------+
| Account Name | Account ID |
+--------------------------+----------------------------------+
| Arv@roci.dev's Account | xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx |
+--------------------------+----------------------------------+
| Hello@roci.dev's Account | xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx |
+--------------------------+----------------------------------+
Error: field `account_id` is required
...but you can use environment variables instead:
CF_ACCOUNT_ID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx wrangler dev
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.
wfm but I only have one account.
No description provided.