-
Notifications
You must be signed in to change notification settings - Fork 20
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 ubiquibot logger support #16
Feat ubiquibot logger support #16
Conversation
This is still a draft pull request. I have to rebase to current development branch, hopefully will be ready for the review tomorrow. |
The ubiquibot-logger integration enables supabase logging for ubiquibot-kernel. On top of SUPABASE_KEY and SUPABASE_URL environment variables, LOG_RETRY_LIMIT and LOG_LEVEL must be specified. Example: LOG_RETRY_LIMIT=3 LOG_LEVEL=DEBUG Resolves: #5
a9867d9
to
0585355
Compare
Hi @pavlovcik @whilefoo I seem to got it right this time -) I rebased the pull request against latest development branch as well. The QA was done on my ubiquibot-kernel fork and supabase instance:
Also tested at gitcoindev/ubiquibot#10 (comment) and received "Hello from the worker" comments. On the way I also bumped wrangler to the latest version 3.25.0 . |
Your test doesn't explicitly show that the logger is working but I trust that you tested that as well |
Yes, I can confirm that. I have logged into my Supabase dashboard and checked that a new row appeared in the |
@@ -1,6 +1,7 @@ | |||
name = "ubiquibot-worker" | |||
main = "src/worker.ts" | |||
compatibility_date = "2023-12-06" | |||
node_compat = true |
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.
Hey @gitcoindev I don't think we can add this
@whilefoo rfc
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.
@pavlovcik @whilefoo I added this config to silence cloudflare worker warning. I decided to dig into this deeper and discovered that this warning may be related to a bug cloudflare/workers-sdk#4050 which was coincidentally put to in progress
40 minutes ago. Let's monitor this and remove once it will be fixed.
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.
But also doesn't our npm library depend on node APIs at runtime? If so we can't actually use it in our production Worker.
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.
ubiquity-logger uses probot's context so that needs to be modified, it uses supabase-js which needs some configuration to work on CF workers (https://github.com/supabase/supabase-js?tab=readme-ov-file#custom-fetch-implementation) and also it uses node's util package
SUPABASE_KEY= | ||
|
||
LOG_RETRY_LIMIT= | ||
LOG_LEVEL= |
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.
Also should include documentation around this. I'm assuming verbose
is fine
Resolves #5