PoC: Add a logging abstraction and wrap remix build output to track errors #941

blittle · 2023-05-24T21:57:48Z

Summary

This PR does two things:

It wraps the Remix module build output. This means that every loader and action method defined in routes now gets automatically wrapped where we call the method before Remix, and catch any errors passing them to a logging abstraction.
Add a lite logging abstraction with a default implementation:

import {Logger} from '@shopify/hydrogen';

// Default log implementation
const log = new Logger(executionContext.waitUntil, request);

// Override specific log options
const log = new Logger(executionContext.waitUntil, request, {
  // Each logger method (trace, debug, warn, error, fatal) is passed a context object,
  // which includes the original request. These methods can also be async functions,
  // which could call a third party API to ingest logs.
  error: ({request}, ...args) => {
    console.log(request.url, ...args);
  }
});

How to test?

Add an error within a loader or action within both the demo store and the skeleton app.
Notice that the skeleton app passes in a Logger instance and the demo store does not. This just verifies that it works with and without the logger passed in. If it isn't passed in, just default to console.error().

github-actions · 2023-05-24T21:58:04Z

We detected some changes in packages/*/package.json or packages/*/src, and there are no updates in the .changeset.
If the changes are user-facing and should cause a version bump, run npm run changeset add to track your changes and include them in the next release CHANGELOG.
If you are making simple updates to examples or documentation, you do not need to add a changeset.

blittle · 2023-05-24T22:01:43Z

packages/remix-oxygen/src/server.ts

  type AppLoadContext,
  type ServerBuild,
 } from '@remix-run/server-runtime';
+import type {Logger} from '@shopify/hydrogen';


CI is failing because of this. I think our build should be more robust, so that our packages can rely on shared types across the monorepo.

Maybe need to update turbo.json to ensure that hydrogen package is built before the remix-oxygen package is built?

Can we just copy the type? I think it would be better if remix-oxygen doesn't depend on Hydrogen... since it's just a generic adapter for Remix <> Oxygen 🤔

Hmm, it might be best to keep them separate

blittle · 2023-05-24T22:02:33Z

packages/remix-oxygen/src/server.ts

-  const handleRequest = createRemixRequestHandler(build, mode);
+  const routes: typeof build.routes = {};
+
+  for (const [id, route] of Object.entries(build.routes)) {


This is where remix modules loader and actions are overwritten

I'm not sure if it's the job of remix-oxygen to wrap the Remix build and enhance it with logs.
Maybe we could also consider exporting this as a standalone function from Hydrogen itself?
Perhaps as part of createLogger (as an option, you pass the Remix build to enhance its logs), or as a separate utility?

Yeah, I was only hoping to automatically add logging for existing merchants without having them update their server.ts. The only way to do that AFAIK is to do it in the oxygen handler. Though maybe that's not a fair tradeoff considering no one outside Oxygen would get it.

@frandiox are you suggesting:

const log = new Logger({ request, waitUntil, remixBuild });

And then having the logger mutate remixBuild.routes?

Either that or as a separate utility, yes. Looking at it now it doesn't feel like it's clear what it does... so maybe a more verbose function name would be better?

blittle · 2023-05-24T22:03:47Z

packages/hydrogen/src/logger.ts

+      if (error instanceof Error) {
+        console.error(red(`Error processing route:${url}\n${error.stack}`));
+      } else {
+        console.error(red(`Error:${url} ${error}`));
+      }


I copied this logic from H1, though not sure if we want to keep it 🤷🏼

I wonder if this is where we implement the logging format that we discussed in #876?

I copied this logic from H1, though not sure if we want to keep it 🤷🏼

Maybe we can remove it for now?

I wonder if this is where we implement the logging format that we discussed in 876

I think that would be a different place since that should only be for Hydrogen logs. This logger here is supposed to be used manually by the user as well, right?

juanpprieto · 2023-05-24T22:26:44Z

packages/remix-oxygen/src/server.ts

@@ -31,6 +47,23 @@ export function createRequestHandler<Context = unknown>({
  };
 }

+type RemixModule = ServerBuild['routes'][0]['module'];
+
+function fill(module: RemixModule, name: 'loader' | 'action', log?: Logger) {


Should it also support handle and meta exports?

frehner

Notice that the skeleton app passes in a Logger instance and the demo store does not. This just verifies that it works with and without the logger passed in. If it isn't passed in, just default to console.error().

I feel like it should be the other way around - implemented in demo-store but not in skeleton? Not a big deal though.

frehner · 2023-05-24T22:23:21Z

packages/hydrogen/src/logger.ts

+
+export class Logger {
+  #waitUntil: (p: Promise<unknown>) => void;
+  #request: Request;


oh baby private fields on a class. We fancy now.

frehner · 2023-05-24T22:24:07Z

packages/hydrogen/src/logger.ts

+type LoggerMethod = (
+  context: {request: Request},
+  ...args: Array<any>
+) => void | Promise<any>;


Promise<void> or do we actually expect to get something out of the promise here?

Probably void is more correct

frehner · 2023-05-24T22:27:12Z

packages/hydrogen/src/logger.ts

+        );
+      }
+    } catch (e) {
+      const message = e instanceof Error ? e.stack : e;


the message is from e.stack? or should it be from e.message?

I think the stack normally contains the same message, so this is just printing the whole thing. Might be wrong tho.

Yeah, the stack contains the message.

frehner · 2023-05-24T22:29:01Z

packages/hydrogen/src/logger.ts

+      if (error instanceof Error) {
+        console.error(red(`Error processing route:${url}\n${error.stack}`));
+      } else {
+        console.error(red(`Error:${url} ${error}`));
+      }


I wonder if this is where we implement the logging format that we discussed in #876?

frehner · 2023-05-24T22:29:53Z

packages/remix-oxygen/src/server.ts

  type AppLoadContext,
  type ServerBuild,
 } from '@remix-run/server-runtime';
+import type {Logger} from '@shopify/hydrogen';


Maybe need to update turbo.json to ensure that hydrogen package is built before the remix-oxygen package is built?

frandiox · 2023-05-25T12:56:46Z

packages/hydrogen/src/logger.ts

@@ -0,0 +1,103 @@
+import {yellow, red} from 'kolorist';


I'd prefer not to bundle kolorist or any library like that in Hydrogen itself. We can just add colors from the CLI in dev by recognizing patterns 🤔

frandiox · 2023-05-25T12:57:48Z

packages/hydrogen/src/logger.ts

+    waitUntil: (p: Promise<unknown>) => void,
+    request: Request,


Maybe request should be the first argument? waitUntil could be optional in some environments like Node.js

frandiox · 2023-05-25T12:59:09Z

packages/hydrogen/src/logger.ts

+        );
+      }
+    } catch (e) {
+      const message = e instanceof Error ? e.stack : e;


I think the stack normally contains the same message, so this is just printing the whole thing. Might be wrong tho.

frandiox · 2023-05-25T13:01:01Z

packages/hydrogen/src/logger.ts

+    trace: (context, ...args) => {
+      console.log(...args);
+    },
+
+    debug: (context, ...args) => {
+      console.log(...args);
+    },


Should these default to console.trace and console.debug respectively?

frandiox · 2023-05-25T13:02:25Z

packages/hydrogen/src/logger.ts

+      if (error instanceof Error) {
+        console.error(red(`Error processing route:${url}\n${error.stack}`));
+      } else {
+        console.error(red(`Error:${url} ${error}`));
+      }


I copied this logic from H1, though not sure if we want to keep it 🤷🏼

Maybe we can remove it for now?

I wonder if this is where we implement the logging format that we discussed in 876

I think that would be a different place since that should only be for Hydrogen logs. This logger here is supposed to be used manually by the user as well, right?

frandiox · 2023-05-25T13:05:34Z

packages/remix-oxygen/src/server.ts

  type AppLoadContext,
  type ServerBuild,
 } from '@remix-run/server-runtime';
+import type {Logger} from '@shopify/hydrogen';


Can we just copy the type? I think it would be better if remix-oxygen doesn't depend on Hydrogen... since it's just a generic adapter for Remix <> Oxygen 🤔

frandiox · 2023-05-25T13:08:54Z

packages/remix-oxygen/src/server.ts

-  const handleRequest = createRemixRequestHandler(build, mode);
+  const routes: typeof build.routes = {};
+
+  for (const [id, route] of Object.entries(build.routes)) {


I'm not sure if it's the job of remix-oxygen to wrap the Remix build and enhance it with logs.
Maybe we could also consider exporting this as a standalone function from Hydrogen itself?
Perhaps as part of createLogger (as an option, you pass the Remix build to enhance its logs), or as a separate utility?

frandiox · 2023-05-26T02:47:19Z

@blittle one more thing to keep in mind: the remixBuild is shared across requests for the same isolate (or same Node.js process). Therefore, we shouldn't modify/wrap it on every request (like new Logger({..., remixBuild}) would do). We'll need to check if it's been already modified and skip it, or create a whole new build object (like I think you are doing in remix-oxygen/src/server.ts).
Therefore, maybe something like const enhancedRemixBuild = addLogsToBuild(remixBuild, logger) would be better? 🤔

frandiox · 2023-05-26T08:33:45Z

@blittle I've pushed a few commits mostly to show that we should also wrap deferred data because Remix also swallows that.

I've also removed type and colors dependencies.

Still not sure if this should happen in remix-oxygen, though.

davidhousedev · 2023-05-26T10:15:25Z

Given that the only extensibility point is the logger method, it requires a more advanced composition pattern (we used higher-order functions) if you want to e.g. send any log statement to an external source.

Have you considered a more generic extension point that would be called for all logger methods?

Also, have you considered introducing a logger.info? It's been confusing for folks on our end adjusting to the idea that "if you want to info log, use debug"

blittle · 2023-05-26T13:58:49Z

@davidhousedev this is an early PoC, and we'd love some ideas. I'm happy to add a .info method.

blittle · 2023-05-26T20:34:17Z

@frandiox we might be able to simplify this given this change to Remix: remix-run/remix#6495

michenly · 2024-02-14T19:30:08Z

@blittle closing this PR due to inactivity. @frandiox unless you have any idea how remix-run/remix#6495 play with our current main and if this PR is still needed?

frandiox · 2024-02-15T01:58:37Z

I don't think we wanted to move forward with this 👍

Add a logging abstraction and wrap remix build output to track errors

9ee7385

blittle commented May 24, 2023

View reviewed changes

blittle requested review from frandiox and wizardlyhel May 24, 2023 22:03

juanpprieto reviewed May 24, 2023

View reviewed changes

frehner reviewed May 24, 2023

View reviewed changes

frandiox reviewed May 25, 2023

View reviewed changes

frandiox added 3 commits May 26, 2023 17:26

Log errors from deferred data

c3f8eee

Inline Logger type

e8ce52b

Remove kolorist

3711b80

github-actions bot deployed to preview May 26, 2023 08:34 View deployment

blittle changed the title ~~Add a logging abstraction and wrap remix build output to track errors~~ PoC: Add a logging abstraction and wrap remix build output to track errors May 26, 2023

michenly closed this Feb 14, 2024

PoC: Add a logging abstraction and wrap remix build output to track errors #941

PoC: Add a logging abstraction and wrap remix build output to track errors #941

Conversation

blittle commented May 24, 2023

Summary

How to test?

github-actions bot commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frehner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frandiox commented May 26, 2023 • edited Loading

frandiox commented May 26, 2023

davidhousedev commented May 26, 2023

blittle commented May 26, 2023

blittle commented May 26, 2023

michenly commented Feb 14, 2024

frandiox commented Feb 15, 2024

frandiox commented May 26, 2023 •

edited

Loading