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

[Feature] Allow creating child loggers, or modifying the parent logger state #331

Open
mcky opened this issue Sep 28, 2023 · 1 comment
Open
Labels
📦 inngest Affects the `inngest` package ✨ new New features, integrations, or exports

Comments

@mcky
Copy link

mcky commented Sep 28, 2023

Is your feature request related to a problem? Please describe.
Inngest's automatic child logger creation is great, but I find myself wanting to pass additional info with each log line, so that they can be grouped together later.

Describe the solution you'd like
I'd like to either be able to

a) Extend the logger from the context by calling .child on it and passing my own context.
It seems like passing a generic inferred from the logger instance passed to the Inngest constructor down to the function could help tell if a logger supports child logging, from the context of the function

  async ({ event, step, logger }) => {
    const transactionLogger = logger.child({ transactionId });

    const model = new SomeModel(deps, transactionLogger);

    transactionLogger.info(`Doing thing for transaction`);

b) Somehow attach more metadata to the function, and/or it's logs (rough sketch as an example, not particularly a suggested API)

  async ({ event, step, logger, setMeta }) => {
    setMeta({ transactionId })
    // all calls to `logger`, if it supports child loggers, will now have the meta applied

Option b is with #330 in mind, although since the middleware run before user code is executed that wouldn't really work.

c) If b doesn't work because of execution order, perhaps being able to call .child from a function-specific middleware

const loggingMiddlewarenew InngestMiddleware({
// ...
          transformInput(ctx) {
            const logger = ctx.logger.child({ transactionId: fromEventDataSomehow })
            return { ctx: { logger } };
          },
});

inngest.createFunction(
  { name: "Example", middleware: [loggingMiddleware] },
  { event: "app/example" },
  async ({ event, step, ctx }) => {
    ctx.logger.info(`I have transactionId in my context`)

Describe alternatives you've considered

  • I can just log the transactionId once, and from that find the inngest provided runID and use that to filter logs
  • I've used pnpm patch to patch the inngest SDK for our app to add hacky support for a child logger. This works for us for now, but it feels very hacky
--- a/middleware/logger.js
+++ b/middleware/logger.js
@@ -26,6 +26,9 @@ class DefaultLogger {
+    child(...args) {
+        throw new Error("logger.child not implemented for DefaultLogger")
+    }
@@ -62,6 +65,9 @@ class ProxyLogger {
+    child(...args) {
+        return __classPrivateFieldGet(this, _ProxyLogger_logger, "f").child(...args);
+    }

Additional context
I found this part of the docs slightly misleading, as it led me to believe I'd be able to bring my own child-compatable logger. Re-reading it I can see it doesn't actually state that, but it tripped me up.

child logger
If the logger library supports a child logger .child() implementation, the built-in middleware will utilize it to add function runtime metadata for you.

@mcky mcky added the ✨ new New features, integrations, or exports label Sep 28, 2023
@jpwilliams
Copy link
Member

Hi, @mcky! 👋

Thanks loads for this detailed write-up.

I believe you should be able to bring your own child-compatible logger if you pass a logger with a .child() when creating your client, but aye, types won't reflect that, meaning you don't have access to it. We could perhaps simply extend whatever logger you pass there, ensuring you have all of the usual methods (like .child()), then this would be less of a problem.

That feels like the best immediate fix, but you raise some good points about middleware and not having access to everything you need. After the fix above, I guess the limiting factor stopping you achieving any of your listed solutions (without patching) is not being able to access logger within the middleware hooks?

The logging itself is, internally, middleware, which currently doesn't appear when creating extra middleware. If you did have access to logger within your middleware, you could shim it nicely and have the ability to add tools you're wanting such as setMeta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 inngest Affects the `inngest` package ✨ new New features, integrations, or exports
Projects
None yet
Development

No branches or pull requests

2 participants