From 11b789d924788abf97460727da18ab75c611b361 Mon Sep 17 00:00:00 2001
From: Noemi <45180344+unflxw@users.noreply.github.com>
Date: Thu, 21 Nov 2024 14:57:20 +0100
Subject: [PATCH] Fix Pino transport

The Pino transport could not be used as a transport (that is, as
initialised by `pino.transport()`, but only as a destination, and
only in the main thread.

This commit makes the Pino transport communicate with the extension
directly, which should work from any worker thread -- the assumption
being that the extension has been started by the main thread.

This works around the fact that the client cannot be serialised and
sent across threads.

This allows for the Pino transport to be used like other transports,
and _alongside_ other transports:

```js
const logger = pino({
  transport: {
    targets: [
      { target: "@appsignal/nodejs/pino", options: { group: "pino" } },
      { target: ... }
    ]
  }
})
```

The `pino/index.js` file has been added as a convenience to allow for
the target to be referred to as `@appsignal/nodejs/pino`, which is
nicer-looking than the transport's real location inside `dist/`.
---
 ...ino-transport-to-be-used-as-a-transport.md | 50 +++++++++++++++++
 .npmignore                                    |  1 +
 pino/index.js                                 |  2 +
 src/pino_transport.ts                         | 55 ++++++++++---------
 4 files changed, 82 insertions(+), 26 deletions(-)
 create mode 100644 .changesets/allow-pino-transport-to-be-used-as-a-transport.md
 create mode 100644 pino/index.js

diff --git a/.changesets/allow-pino-transport-to-be-used-as-a-transport.md b/.changesets/allow-pino-transport-to-be-used-as-a-transport.md
new file mode 100644
index 00000000..903cfedd
--- /dev/null
+++ b/.changesets/allow-pino-transport-to-be-used-as-a-transport.md
@@ -0,0 +1,50 @@
+---
+bump: patch
+type: fix
+---
+
+Allow Pino transport to be used as a transport. Before, our Pino transport could only be used as a destination:
+
+```js
+import pino from "pino";
+import { Appsignal, AppsignalPinoTransport } from "@appsignal/nodejs";
+
+pino(AppsignalPinoTransport({
+  client: Appsignal.client,
+  group: "pino"
+}));
+```
+
+This meant it was not possible to log both to our transport and to another destination.
+
+Now, it is also possible to use it as a Pino transport, with the `transport` Pino config option or the `pino.transport()` function:
+
+```js
+import pino from "pino";
+
+pino({
+  transport: {
+    target: "@appsignal/nodejs/pino",
+    options: {
+      group: "pino"
+    }
+  }
+});
+```
+
+It is no longer necessary to pass the AppSignal client to the Pino transport. AppSignal must be active for the Pino transport to work.
+
+By enabling its use as a transport, it is now possible to use it alongside other transports:
+
+```js
+pino({
+  transport: {
+    targets: [
+      // Send logs to AppSignal...
+      { target: "@appsignal/nodejs/pino" },
+      // ... and to standard output!
+      { target: "pino/file" }
+    ]
+  }
+});
+```
diff --git a/.npmignore b/.npmignore
index 15186f27..80dff810 100644
--- a/.npmignore
+++ b/.npmignore
@@ -1,5 +1,6 @@
 *
 !/dist/**/*
+!/pino/**/*
 !/scripts/**/*
 scripts/**/*.test.js
 !/ext/appsignal_extension.cpp
diff --git a/pino/index.js b/pino/index.js
new file mode 100644
index 00000000..fcfdf38f
--- /dev/null
+++ b/pino/index.js
@@ -0,0 +1,2 @@
+"use strict"
+module.exports = require("../dist/pino_transport")
diff --git a/src/pino_transport.ts b/src/pino_transport.ts
index 3d60ce1d..f545c73a 100644
--- a/src/pino_transport.ts
+++ b/src/pino_transport.ts
@@ -1,44 +1,47 @@
-import { LOGGER_LEVEL_SEVERITY } from "./logger"
+import { LOGGER_LEVEL_SEVERITY, LOGGER_FORMAT } from "./logger"
 import build from "pino-abstract-transport"
-import { Client } from "./client"
+import { Extension } from "./extension"
 
 type PinoTransportOptions = {
-  client: Client
-  group: string
+  group?: string
 }
 
-const appsignalPinoTransport = ({
-  client,
-  group = "app"
-}: PinoTransportOptions) => {
+type LogData = {
+  severity: number
+  message: string
+  attributes: Record<string, any>
+}
+
+const appsignalPinoTransport = ({ group = "app" }: PinoTransportOptions) => {
   return build(async (source: any) => {
+    // We expect this code to be running in a worker thread and for the
+    // extension to have already been loaded and started on the main thread.
+    // Since we expect the extension to already have been initialised, we
+    // pass `{ active: false }` to avoid initialising it again.
+    const extension = new Extension({ active: false })
+
     for await (const obj of source) {
-      sendLogs(parseInfo(obj, group), client)
+      sendLogs(extension, group, parseInfo(obj))
     }
   })
+}
 
-  async function sendLogs(data: Record<string, any>, client: Client) {
-    client.extension.log(
-      data.group || "app",
-      data.severity,
-      0,
-      data.msg,
-      data.attributes
-    )
-  }
+async function sendLogs(extension: Extension, group: string, data: LogData) {
+  extension.log(
+    group,
+    data.severity,
+    LOGGER_FORMAT.plaintext,
+    data.message,
+    data.attributes
+  )
 }
 
-function parseInfo(
-  obj: Record<string, any>,
-  group: string
-): Record<string, any> {
-  const { hostname, level, msg, ...attributes } = obj
+function parseInfo(obj: Record<string, any>): LogData {
+  const { level, msg, ...attributes } = obj
 
   return {
     severity: getSeverity(level),
-    hostname,
-    group,
-    msg,
+    message: msg,
     attributes: flattenAttributes(attributes)
   }
 }