Restructuring log middleware

[wisko]

No description provided.

[wisko]

Maybe we should combine these rows, since we might get no trace if the header was invalid. Thoughts?

[MattiasOlla]

I think both variants work.

[wisko]

I'll leave it like this for now then 😄
Probably easier to notice somethings wrong if the trace doesn't exist vs some random other trace is there instead.

[wisko]

Perhaps also rename this to something better too. Prefix with create? Or should it be called something less generic over all - app.use(logStoreMiddleware()) or app.use(logTracingMiddleware())?

[assensamuelsson]

I think middleware is ok. We're only exporting 1 middleware in this lib and callers are free to rename it when they import it.

[wisko]

Perfect! I've always felt torn on this topic, since named exports affect implementors this way. But I'm happy with this approach.

[wisko]

One thing that I changed was to remove spanId as its own log field, since its value already existed in logging.googleapis.com/spanId (unlike the traceId). I can bring it back if we want? Otherwise I just need to update some tests.

[MattiasOlla]

I'm not sure we need this as a separate function. Also, in this form, you can only add static data to your logs, which might not be that interesting. My thought was to add an optional argument to the middleware factory function, that can add logging data from the Request object. something like:

type MiddlewareConfig = {
  extractRequestData?: (request: Request) => Record<string, unknown>;
};

export const middleware: Middleware = ({ extractRequestData }: MiddlewareConfig = {}) => {
  return (req, _res, next) => {
    ...

    if (extractRequestData) {
      extra = extractRequestData(req);
      for (const key in obj) {
        logFields[key] = obj[key];
      }
    }
    ...

This way, you also don't have to keep track of whether the storage has been initalized or not, it's all done in the middleware.

[MattiasOlla]

My specific use case in greenfield is extracting the IAP headers to log which service account made a specific request.

[wisko]

I agree that type of middleware option function is great! But I think its nice to have a separate decorateLogs type function that you can use anywhere in your application, such as if (user) { decorateLogs(...) }. Lets att that middleware option in the next PR 🙏

I can remove the extra function 👌

[wisko]

Wait, I misunderstood. I thought you were referring to another function. I think separating the argument on the middleware from decorateLogs() could be nice, since they cover different use cases.

[MattiasOlla]

Ah, so the idea is to write my own middleware where decorateLogs is called? That is probably even more flexible actually.

[wisko]

Yes, you can either call it inside your own middleware, or anywhere in the request flow. I have used it on Di's site in some conditional logic before.

[MattiasOlla]

I think both variants work.

[MattiasOlla]

One thing that I changed was to remove spanId as its own log field, since its value already existed in logging.googleapis.com/spanId (unlike the traceId). I can bring it back if we want? Otherwise I just need to update some tests.

The only reason to keep it as a separate field is to log it properly if we run outside GCP, so maybe add it back if we don't have a gcpProjectId?

[wisko]

One thing that I changed was to remove spanId as its own log field, since its value already existed in logging.googleapis.com/spanId (unlike the traceId). I can bring it back if we want? Otherwise I just need to update some tests.

The only reason to keep it as a separate field is to log it properly if we run outside GCP, so maybe add it back if we don't have a gcpProjectId?

Ah, of course! I will do that 🥳

[assensamuelsson]

I like this approach a lot. Have my 👍

What do you think about always logging spanId even for GCP services? This is how we described the standard in the technical principles PR right now. It is somewhat redundant but keeps message structure consistent regardless of where the application is hosted.

[MattiasOlla]

Vi kör!