vikejs / vike

🔨 Flexible, lean, community-driven, dependable, fast Vite-based frontend framework.
https://vike.dev
MIT License
4.37k stars 350 forks source link

Make logs configurable #1013

Closed brillout closed 10 months ago

brillout commented 1 year ago

Description

E.g. https://vitejs.dev/config/shared-options.html#customlogger.

crummy commented 1 year ago

I didn't realise Vite already has this functionality. Seems like Vike has its own custom logger however. Is it worth doing this in e.g. log.ts:

const log = createLogger() // access Vite's logger
function logDirectly(thing: unknown, logType: LogType) {
  applyViteSourceMapToStackTrace(thing)
  isFirstLog = false
  if (logType === 'info') {
    log.info(thing) // use Vite's logger, not console.log
    return
  }
  [...etc]
}

This would let us use the existing Vite log filtering.

brillout commented 1 year ago

The problem with that is that Vite's logger is only available in development. In production Vite doesn't even exist.

crummy commented 1 year ago

For me Vike in production is just static files so I'm not familiar with how logging is used in prod. Would it be suitable to use Vite logging for dev only in Vike?

brillout commented 1 year ago

Being able to handle logging in production is quite a must have for SSR apps.

brillout commented 1 year ago

FYI it's probably better I implement this, it isn't as easy as it seems.

Victor2333 commented 1 year ago

As work around for me. I use pino and pino-http. pino-http will generate log instance to express request. Here's my example:

// .client.ts
export const logger = () => pino();
// .server.ts
export const logger = (pageContext:PageContext) => pageContext.req.log;
// .page.ts
export const Page = () => {
  const logger = useLogger()
  logger.info("test")
}
// useLoggerHook
export function useLogger() {
  const pageContext = usePageContext();
  const {
    exports: { logger },
  } = pageContext;
  return logger(pageContext);
}
// /index.server.ts
  router.get('*', async (req, res, next) => {
    const pageContextInit = {
      req,
      urlOriginal: req.originalUrl,
    };
    const pageContext = await renderPage(pageContextInit);
    const { httpResponse } = pageContext;
    if (!httpResponse) return next();
    const { body, statusCode, contentType, earlyHints } = httpResponse;
    if (res.writeEarlyHints)
      res.writeEarlyHints({ link: earlyHints.map((e) => e.earlyHintLink) });
    res.status(statusCode).type(contentType).send(body);
  });
Victor2333 commented 1 year ago

FYI it's probably better I implement this, it isn't as easy as it seems.

I don’t think vps need implement log logic. Log should be implement by third party library or custom code. vps only need handle vps log. I think a better idea is use custom export. And implement custom function both server side and client side case.

For vite customLogger. It only working in node env. So in client side, I think we shouldn’t use it.

brillout commented 1 year ago

Plan so far:

function onLogServer(log: {
  msg: string
  type: 'error' | 'warning' | 'info'
  print: boolean
  // Original error that triggered the error log
  err?: Error
  // msgRest is null if the the log is a one-liner, otherwise: msg contains the first line while msgRest
  // contains all subsequent lines.
  msgRest: null | string
}): undefind | boolean {
  if (someCondition && log.print) {
    // Swallow
    return false
  }

  // Verbose
  if (!log.print) {
    // Always show
    return true
  }
}

Same for onLogClient().

chrisvander commented 1 year ago

I'd like to be able to feed logs through Winston and then to my Datadog instance, so this is a must. Seconded! Would registering a callback make the most sense here? The only thing I use from VPS on the server is the renderPage() call, so would I pass an onLogServer callback to replace VPS's default?

brillout commented 1 year ago

Would registering a callback make the most sense here?

Some logs are logged before any renderPage() is called, so it has to be seen how we can enable the user to intercept logs as soon as possible.

would I pass an onLogServer callback to replace VPS's default

See my previous comment: if you return false then you essentially overwrite VPS's logging.

I'd like to be able to feed logs through Winston and then to my Datadog instance, so this is a must.

In the meantime, you can intercept JavaScript's console.log() — there are plenty of examples of how to acheive that.

brillout commented 1 year ago

I'm actually wondering: as a user, I cannot assume that all the libraries I use provide log hooks, so, if I want to reliably intercept all logs, then I have to intercept console.log().

From that perspective, this feature request is actually obsolete... Although this assumes that there are some neat libraries out there to easily intercept console.log(). (The library could even detect which npm package is triggering the log.)

(That said, a way to intercept errors is still needed because there is no way to intercept errors. Or actually maybe there is by intercepting the Error object; I never tried but it would an interesting experiment. Possible issue: is it possible to detect whether the error was catched?)

chrisvander commented 1 year ago

I tend to assume that there's either no logging (e.g. deprecation notices in the IDE and other methods of providing warnings) or a method to configure logging. Just intercepting console.log() puts it on the end user to also figure out:

If we're talking about something as big as an SSR framework, I wouldn't want logs during production to be polluted with messages of varying formats and at various levels. If anything, my alternative is to disable logging altogether from VPS, but that would decrease my visibility into that part of the system.

chrisvander commented 1 year ago

Also, I don't think recommended practice includes modifying Node.JS native components, like console.log. It's probably better to have some log function that everything in VPS gets routed through, and then by default, the formatting is applied and it's printed to the appropriate console.log or console.warn.

brillout commented 1 year ago

In principle, I'd agree with you.

But, in practice, the fact that only very few users are asking for this feature speaks volume about how log interecpting is usually(/almost ubiquitously?) done in JavaScript. E.g. Sentry also intercepts console.log(). Yes, it's ugly, but it works.

Also, I don't think recommended practice includes modifying Node.JS native components, like console.log.

I definitely agree on that for npm packages (and vite-plugin-ssr is meticulous about interfering with built-ins as less as possible and only if strictly needed). But doing it on the user land is okay-ish, as attested by the many who do it.

To be clear: I'm open for implementing this feature, it's just that I'll prioritize it depending on concrete use cases that demand it.

Let me know if you run into concrete issues intercepting console.log().

chrisvander commented 1 year ago

It works for the time being. I'm intercepting using an async function around the renderPage call, and filtering out logs that I don't need to see. The parsing out timestamp and leading information in the message might be flaky but works for now! This feature would be really great, but it doesn't have to be prioritized for v1.

brillout commented 1 year ago

Vite-plugin-ssr purposely doesn't show information logs in production (in order to avoid cluttering the user's production logs).

One use case that intercepting console.log() cannot provide is the ability to add these information logs to the user's production logs collector. Alternative: a config verboseProductionLogs: boolean, but that isn't granular.

So, yea, this feature request is still a consideration. But low priority.

brillout commented 10 months ago

Closing in favor of https://github.com/vikejs/vike/issues/1438.