Fix visibility of logs send to OTEL (#1577)

.changeset/many-tigers-crash.md

@@ -0,0 +1,5 @@
+---
+"@saleor/apps-otel": patch
+---
+
+Patch OTEL dependency - it should handle NaN & Infinity values. It will ensure that logs are properly parsed and send to log service.

.changeset/thick-poets-invite.md

@@ -0,0 +1,5 @@
+---
+"@saleor/apps-logger": patch
+---
+
+Changed how we serialize errors in logs for OTEL. After this change they will be JSON stringified instead on modern error serialize function which didn't work with nested objects.

apps/avatax/src/lib/app-config-extractor.ts

@@ -47,7 +47,9 @@ export class AppConfigExtractor implements IAppConfigExtractor {
     );
 
     if (appConfigResult.isErr()) {
-      this.logger.error("Failed to resolve app configuration", { error: appConfigResult.error });
+      this.logger.error("Failed to resolve app configuration", {
+        error: appConfigResult.error,
+      });
 
       return err(
         new AppConfigExtractor.CantResolveAppConfigError("Failed to extract config from metadata", {

apps/avatax/src/lib/app-configuration-logger.test.ts

@@ -81,13 +81,13 @@ describe("AppConfigurationLogger", () => {
     );
 
     expect(mockInfo).toHaveBeenCalledWith("Received configuration", {
-      address: {
+      address: JSON.stringify({
         city: "test",
         country: "test",
         zip: "10111",
         state: "NY",
         street: "test",
-      },
+      }),
       appConfigName: "config",
       channelSlug: "default-channel",
       companyCode: "test",

apps/avatax/src/lib/app-configuration-logger.ts

@@ -29,7 +29,7 @@ export class AppConfigurationLogger {
       appConfigName: resolvedAvataxConfig.config.name,
       shippingTaxCode: resolvedAvataxConfig.config.shippingTaxCode,
       companyCode: resolvedAvataxConfig.config.companyCode,
-      address: resolvedAvataxConfig.config.address,
+      address: JSON.stringify(resolvedAvataxConfig.config.address),
       isSandbox: resolvedAvataxConfig.config.isSandbox,
       isAutocommit: resolvedAvataxConfig.config.isAutocommit,
       isDocumentRecordingEnabled: resolvedAvataxConfig.config.isDocumentRecordingEnabled,

apps/avatax/src/modules/app/order-metadata-manager.ts

@@ -51,15 +51,17 @@ export class OrderMetadataManager {
     const errorToReport = error ?? gqlErrors[0] ?? null;
 
     if (errorToReport) {
-      this.logger.error("Failed to update metadata", {
-        error: new OrderMetadataManager.MutationError(
-          errorToReport.message ?? "Failed to update metadata",
-          {
-            props: {
-              error: errorToReport,
-            },
+      const error = new OrderMetadataManager.MutationError(
+        errorToReport.message ?? "Failed to update metadata",
+        {
+          props: {
+            error: errorToReport,
           },
-        ),
+        },
+      );
+
+      this.logger.error("Failed to update metadata", {
+        error,
       });
 
       throw new OrderMetadataManager.MutationError("Failed to update metadata", {

apps/avatax/src/modules/avatax/avatax-client-tax-code.service.ts

@@ -38,11 +38,13 @@ export class AvataxClientTaxCodeService {
         ...(filter ? { filter: `taxCode contains "${filter}"` } : {}),
         top: 50,
       })
-      .catch((err) => {
-        this.logger.error("Failed to call listTaxCodes on Avatax client", { error: err });
+      .catch((error) => {
+        this.logger.error("Failed to call listTaxCodes on Avatax client", {
+          error,
+        });
 
         try {
-          const parsedError = AvataxErrorShape.parse(err);
+          const parsedError = AvataxErrorShape.parse(error);
 
           /*
            * Catch specific client error so it's returned to the frontend
@@ -52,13 +54,13 @@ export class AvataxClientTaxCodeService {
             throw new AvataxClientTaxCodeService.ForbiddenAccessError(
               "PermissionRequired error was returned from Avatax",
               {
-                cause: err,
+                cause: error,
               },
             );
           }
 
           // Throw other errors like usual
-          throw err;
+          throw error;
         } catch (outerError) {
           throw outerError;
         }

apps/avatax/src/modules/avatax/extract-transaction-redacted-log-properties.ts

@@ -16,7 +16,7 @@ export const extractTransactionRedactedLogProperties = (model: CreateTransaction
     description: line.description,
     discounted: line.discounted,
   })),
-  date: model.date,
+  date: model.date.toISOString(),
   isTaxIncluded: model.lines[0]?.taxIncluded ?? false,
   discountAmount: model.discount,
 });

apps/avatax/src/modules/avatax/tax-code/avatax-tax-codes.router.ts

@@ -42,8 +42,8 @@ export const avataxTaxCodesRouter = router({
       ),
     );
 
-    const connection = await connectionService.getById(input.connectionId).catch((err) => {
-      logger.error("Failed to resolve connection from settings", { error: err });
+    const connection = await connectionService.getById(input.connectionId).catch((error) => {
+      logger.error("Failed to resolve connection from settings", { error: error });
 
       throw new TRPCError({
         message: "Failed to resolve configuration, please try again",
@@ -61,10 +61,10 @@ export const avataxTaxCodesRouter = router({
 
     logger.debug("Returning tax codes");
 
-    return taxCodesService.getAllFiltered({ filter: input.filter }).catch((err) => {
-      logger.error("Failed to fetch tax codes from AvaTax", { error: err });
+    return taxCodesService.getAllFiltered({ filter: input.filter }).catch((error) => {
+      logger.error("Failed to fetch tax codes from AvaTax", { error: error });
 
-      if (err instanceof AvataxClientTaxCodeService.ForbiddenAccessError) {
+      if (error instanceof AvataxClientTaxCodeService.ForbiddenAccessError) {
         throw new TRPCError({
           code: "FORBIDDEN",
           cause: "AvaTax PermissionRequired",

apps/avatax/src/modules/avatax/tax-code/avatax-tax-codes.service.ts

@@ -23,10 +23,10 @@ export class AvataxTaxCodesService {
   }
 
   async getAllFiltered({ filter }: { filter: string | null }): Promise<TaxCode[]> {
-    const response = await this.client.getFilteredTaxCodes({ filter }).catch((err) => {
-      this.logger.error("Failed to fetch filtered tax codes", { error: err });
+    const response = await this.client.getFilteredTaxCodes({ filter }).catch((error) => {
+      this.logger.error("Failed to fetch filtered tax codes", { error });
 
-      throw err;
+      throw error;
     });
 
     return this.adapt(response);

apps/avatax/src/modules/calculate-taxes/use-case/calculate-taxes.use-case.ts

@@ -106,7 +106,9 @@ export class CalculateTaxesUseCase {
     const config = this.extractConfig(appMetadata, channelSlug);
 
     if (config.isErr()) {
-      this.logger.warn("Failed to extract app config from metadata", { error: config.error });
+      this.logger.warn("Failed to extract app config from metadata", {
+        error: config.error,
+      });
 
       ClientLogStoreRequest.create({
         level: "error",
@@ -153,7 +155,7 @@ export class CalculateTaxesUseCase {
         }
         default: {
           Sentry.captureException(innerError);
-          this.logger.fatal("Unhandled error", { error: err });
+          this.logger.fatal("Unhandled error", { error: innerError });
 
           return err(
             new CalculateTaxesUseCase.UnhandledError("Unhandled error", { errors: [innerError] }),
@@ -225,7 +227,7 @@ export class CalculateTaxesUseCase {
         });
       },
     ).map((results) => {
-      this.logger.info("Taxes calculated", { calculatedTaxes: results });
+      this.logger.info("Taxes calculated", { calculatedTaxes: JSON.stringify(results) });
 
       ClientLogStoreRequest.create({
         level: "info",

apps/avatax/src/modules/crud-settings/crud-settings.service.ts

@@ -46,7 +46,7 @@ export class CrudSettingsManager {
 
     if (!validation.success) {
       this.logger.error("Error while validating metadata", {
-        error: validation.error,
+        error: JSON.stringify(validation.error),
         metadataKey: this.params.metadataKey,
       });
       throw new Error("Error while validating metadata");

apps/avatax/src/modules/tax-classes/tax-classes.router.ts

@@ -15,8 +15,8 @@ export const taxClassesRouter = router({
 
     logger.debug("Returning tax classes");
 
-    return taxClassesFetcher.fetch().catch((err) => {
-      logger.error("Failed to fetch tax classes", { error: err });
+    return taxClassesFetcher.fetch().catch((error) => {
+      logger.error("Failed to fetch tax classes", { error: error });
 
       // TODO: Map errors from Saleor and return proper response
       throw new TRPCError({

apps/avatax/src/pages/api/webhooks/checkout-calculate-taxes.ts

@@ -47,7 +47,7 @@ export default wrapWithLoggerContext(
           subscriptionErrorChecker.checkPayload(payload);
 
           logger.info("Tax base payload for checkout calculate taxes", {
-            payload: payload.taxBase,
+            payload: JSON.stringify(payload.taxBase),
           });
 
           loggerContext.set(ObservabilityAttributes.CHANNEL_SLUG, ctx.payload.taxBase.channel.slug);
@@ -88,24 +88,24 @@ export default wrapWithLoggerContext(
             });
 
           if (config.isErr()) {
-            logger.warn("Failed to extract app config from metadata", { error: config.error });
+            logger.warn("Failed to extract app config from metadata", {
+              error: config.error,
+            });
 
             return res.status(400).json({
               message: `App configuration is broken for checkout: ${payload.taxBase.sourceObject.id}`,
             });
           }
 
-          metadataCache.setMetadata(appMetadata);
-
           return useCase.calculateTaxes(payload, authData).then((result) => {
             return result.match(
               (value) => {
                 return res.status(200).json(ctx.buildResponse(value));
               },
-              (err) => {
-                logger.warn("Error calculating taxes", { error: err });
+              (error) => {
+                logger.warn("Error calculating taxes", { error });
 
-                switch (err.constructor) {
+                switch (error.constructor) {
                   case CalculateTaxesUseCase.FailedCalculatingTaxesError: {
                     return res.status(500).json({
                       message: `Failed to calculate taxes for checkout: ${payload.taxBase.sourceObject.id}`,
@@ -122,7 +122,7 @@ export default wrapWithLoggerContext(
                     });
                   }
                   case CalculateTaxesUseCase.UnhandledError: {
-                    captureException(err);
+                    captureException(error);
 
                     return res.status(500).json({
                       message: `Failed to calculate taxes (Unhandled error) for checkout: ${payload.taxBase.sourceObject.id}`,

apps/avatax/src/pages/api/webhooks/order-calculate-taxes.ts

@@ -105,7 +105,9 @@ export default wrapWithLoggerContext(
             });
 
           if (config.isErr()) {
-            logger.warn("Failed to extract app config from metadata", { error: config.error });
+            logger.warn("Failed to extract app config from metadata", {
+              error: config.error,
+            });
 
             ClientLogStoreRequest.create({
               level: "error",
@@ -122,8 +124,6 @@ export default wrapWithLoggerContext(
             });
           }
 
-          metadataCache.setMetadata(appMetadata);
-
           const AvataxWebhookServiceFactory = await import(
             "../../../modules/taxes/avatax-webhook-service-factory"
           ).then((m) => m.AvataxWebhookServiceFactory);
@@ -161,7 +161,7 @@ export default wrapWithLoggerContext(
             );
 
             // eslint-disable-next-line @saleor/saleor-app/logger-leak
-            logger.info("Taxes calculated", { calculatedTaxes });
+            logger.info("Taxes calculated", { calculatedTaxes: JSON.stringify(calculatedTaxes) });
 
             ClientLogStoreRequest.create({
               level: "info",
@@ -202,7 +202,7 @@ export default wrapWithLoggerContext(
               }
               default: {
                 Sentry.captureException(avataxWebhookServiceResult.error);
-                logger.fatal("Unhandled error", { error: err });
+                logger.error("Unhandled error", { error: err });
 
                 ClientLogStoreRequest.create({
                   level: "error",

cspell.json

@@ -101,6 +101,7 @@
     "**/*.bru",
     "**/.eslintrc*",
     ".github/**",
-    "Dockerfile*"
+    "Dockerfile*",
+    "patches/**"
   ]
 }

package.json

@@ -64,6 +64,10 @@
   "pnpm": {
     "overrides": {
       "crypto-js@<4.2.0": ">=4.2.0"
+    },
+    "patchedDependencies": {
+      "@opentelemetry/[email protected]": "patches/@[email protected]",
+      "@opentelemetry/[email protected]": "patches/@[email protected]"
     }
   },
   "private": true,

packages/logger/src/logger-otel-transport.ts

@@ -2,6 +2,7 @@ import { context } from "@opentelemetry/api";
 import { LogAttributeValue, logs } from "@opentelemetry/api-logs";
 import { SemanticResourceAttributes } from "@opentelemetry/semantic-conventions";
 import { ILogObj, Logger } from "tslog";
+
 import { LoggerContext } from "./logger-context";
 
 export const attachLoggerOtelTransport = (
@@ -45,17 +46,9 @@ export const attachLoggerOtelTransport = (
      * Try to serialize Error. Modern-errors has plugin to serialize
      * https://github.com/ehmicky/modern-errors-serialize
      *
-     * It add "serialize" method that converts class to plain object, working for OTEL.
-     *
-     * This is not perfect, doesn't work for nested object. We probably need to introduce some abstraction
-     * on logger error?
      */
     try {
-      const errorAttribute = serializedAttributes.error;
-      const ErrorConstructor = errorAttribute["constructor"];
-
-      // @ts-expect-error - ErrorConstructor is a class that could have serialize method. If not, safely throw and ignore
-      serializedAttributes.error = ErrorConstructor.serialize(serializedAttributes.error);
+      serializedAttributes.error = JSON.stringify(serializedAttributes.error);
       // @ts-expect-error - Additional mapping for Datadog
       serializedAttributes.error.type = serializedAttributes.error.name;
     } catch (e) {}

packages/logger/src/logger.test.ts

@@ -96,20 +96,21 @@ describe("Logger", () => {
     });
 
     it("Calls Open Telemetry logger emit() function, passing there error attribute", () => {
-      expect.assertions(3);
+      expect.assertions(2);
 
       const logger = createLogger("Test Logger", {
         rootScopePrimitiveArg: 1,
         rootScopeObjectArg: {
           objectKey: "objectValue",
         },
+        error: new Error("Error Message"),
       });
 
       const mockOtelEmit = vi.fn().mockImplementation((log) => {
         const error = log.attributes.error;
 
-        expect(error.message).toBe("Error Message");
-        expect(error.cause).toBe("Error cause");
+        // Error is serialized to JSON
+        expect(error).toBe("{}");
       });
 
       vi.spyOn(logs, "getLogger").mockImplementation(() => {
@@ -139,7 +140,7 @@ describe("Logger", () => {
         context: expect.anything(), // Unique otel context
         body: "[Test Logger] Test Message",
         attributes: {
-          error: expect.any(Error),
+          error: expect.any(String),
           rootScopePrimitiveArg: 1,
           rootScopeObjectArg: {
             objectKey: "objectValue",