Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging to Logtail inside the Netlify update-settings function #1265

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Logging to Logtail inside the Netlify update-settings function #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions web/netlify/functions/update-settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ import { createClient } from "@supabase/supabase-js";
import { Database } from "../../src/types/supabase-notification";
import messages from "../../src/consts/eip712-messages";
import { EMAIL_REGEX, TELEGRAM_REGEX, ETH_ADDRESS_REGEX, ETH_SIGNATURE_REGEX } from "../../src/consts/index";
import { createLogger, throwNewError } from "../../src/utils/logger";
import dotenv from "dotenv";

dotenv.config();

type NotificationSettings = {
email?: string;
Expand All @@ -13,12 +17,15 @@ type NotificationSettings = {
signature: string;
};

const logger = createLogger(process.env.LOGTAIL_TOKEN).child({ function: "update-settings" });
const logAndThrowNewError = (message: string, error?: any) => throwNewError(logger, message, error);
Comment on lines +20 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure LOGTAIL_TOKEN is defined and handle missing token cases

The LOGTAIL_TOKEN environment variable is used to initialize the logger without a check for its existence. If LOGTAIL_TOKEN is undefined, createLogger may fail or behave unexpectedly. To prevent potential runtime errors, consider adding a check to handle missing or invalid LOGTAIL_TOKEN values.

Apply this diff to add the check:

+if (!process.env.LOGTAIL_TOKEN) {
+  throw new Error("LOGTAIL_TOKEN environment variable is not set.");
+}
 const logger = createLogger(process.env.LOGTAIL_TOKEN).child({ function: "update-settings" });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const logger = createLogger(process.env.LOGTAIL_TOKEN).child({ function: "update-settings" });
const logAndThrowNewError = (message: string, error?: any) => throwNewError(logger, message, error);
if (!process.env.LOGTAIL_TOKEN) {
throw new Error("LOGTAIL_TOKEN environment variable is not set.");
}
const logger = createLogger(process.env.LOGTAIL_TOKEN).child({ function: "update-settings" });
const logAndThrowNewError = (message: string, error?: any) => throwNewError(logger, message, error);


const parse = (inputString: string): NotificationSettings => {
let input;
try {
input = JSON.parse(inputString);
} catch (err) {
throw new Error("Invalid JSON format");
logAndThrowNewError("Invalid JSON format", err);
}

const requiredKeys: (keyof NotificationSettings)[] = ["nonce", "address", "signature"];
Expand All @@ -27,37 +34,37 @@ const parse = (inputString: string): NotificationSettings => {

for (const key of requiredKeys) {
if (!receivedKeys.includes(key)) {
throw new Error(`Missing key: ${key}`);
logAndThrowNewError(`Missing key: ${key}`);
}
}

const allExpectedKeys = [...requiredKeys, ...optionalKeys];
for (const key of receivedKeys) {
if (!allExpectedKeys.includes(key as keyof NotificationSettings)) {
throw new Error(`Unexpected key: ${key}`);
logAndThrowNewError(`Unexpected key: ${key}`);
}
}

const email = input.email ? input.email.trim() : "";
if (email && !EMAIL_REGEX.test(email)) {
throw new Error("Invalid email format");
logAndThrowNewError("Invalid email format");
}

const telegram = input.telegram ? input.telegram.trim() : "";
if (telegram && !TELEGRAM_REGEX.test(telegram)) {
throw new Error("Invalid Telegram username format");
logAndThrowNewError("Invalid Telegram username format");
}

if (!/^\d+$/.test(input.nonce)) {
throw new Error("Invalid nonce format. Expected an integer as a string.");
logAndThrowNewError("Invalid nonce format. Expected an integer as a string.");
}

if (!ETH_ADDRESS_REGEX.test(input.address)) {
throw new Error("Invalid Ethereum address format");
logAndThrowNewError("Invalid Ethereum address format");
}

if (!ETH_SIGNATURE_REGEX.test(input.signature)) {
throw new Error("Invalid signature format");
logAndThrowNewError("Invalid signature format");
}

return {
Expand All @@ -72,7 +79,7 @@ const parse = (inputString: string): NotificationSettings => {
export const handler: Handler = async (event) => {
try {
if (!event.body) {
throw new Error("No body provided");
logAndThrowNewError("No body provided");
}
const { email, telegram, nonce, address, signature } = parse(event.body);
const lowerCaseAddress = address.toLowerCase() as `0x${string}`;
Expand All @@ -85,6 +92,7 @@ export const handler: Handler = async (event) => {
});
if (!isValid) {
// If the recovered address does not match the provided address, return an error
logAndThrowNewError("Signature verification failed");
throw new Error("Signature verification failed");
Comment on lines +95 to 96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove redundant throw after logAndThrowNewError

The logAndThrowNewError function already throws an error after logging. The subsequent throw new Error("Signature verification failed"); will not be reached and is redundant.

Apply this diff to remove the redundant throw statement:

   logAndThrowNewError("Signature verification failed");
-  throw new Error("Signature verification failed");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logAndThrowNewError("Signature verification failed");
throw new Error("Signature verification failed");
logAndThrowNewError("Signature verification failed");

}

Expand All @@ -94,6 +102,7 @@ export const handler: Handler = async (event) => {
if (email === "" && telegram === "") {
const { error } = await supabase.from("users").delete().match({ address: lowerCaseAddress });
if (error) throw error;
logger.info("Record deleted successfully.");
return { statusCode: 200, body: JSON.stringify({ message: "Record deleted successfully." }) };
}

Expand All @@ -105,8 +114,12 @@ export const handler: Handler = async (event) => {
if (error) {
throw error;
}
logger.info("Record updated successfully.");
return { statusCode: 200, body: JSON.stringify({ message: "Record updated successfully." }) };
} catch (err) {
logger.error(err);
return { statusCode: 500, body: JSON.stringify({ message: `Error: ${err}` }) };
} finally {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid returning internal error details in HTTP responses

Returning internal error objects directly to the client can expose sensitive information or implementation details. It's safer to provide a generic error message to the client while logging the detailed error internally.

Apply this diff to return a generic error message:

 logger.error(err);
-return { statusCode: 500, body: JSON.stringify({ message: `Error: ${err}` }) };
+return { statusCode: 500, body: JSON.stringify({ message: "An unexpected error occurred." }) };

Committable suggestion was skipped due to low confidence.

logger.flush();
}
};
3 changes: 3 additions & 0 deletions web/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
"@filebase/client": "^0.0.5",
"@kleros/kleros-v2-contracts": "workspace:^",
"@kleros/ui-components-library": "^2.6.2",
"@logtail/pino": "^0.4.12",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Unused logging dependencies detected

It appears that "@logtail/pino" is added as a dependency but is not being utilized in the codebase. Additionally, "pino-pretty" is included but not referenced in any npm scripts or configuration files.

Please verify whether these dependencies are intended to be used and ensure they are properly integrated. If they are not required, consider removing them to maintain a clean and efficient dependency list.

🔗 Analysis chain

New logging dependencies added

The addition of "@logtail/pino", "pino", and "pino-pretty" dependencies indicates an enhancement in the project's logging capabilities. This aligns with the PR objectives of improving error handling and logging.

A few points to consider:

  1. Ensure that these new dependencies are compatible with the existing project setup.
  2. Verify that the chosen versions are stable and appropriate for the project's needs.
  3. Update any relevant documentation to reflect the new logging system.
  4. Consider adding a brief comment in the package.json file explaining the purpose of these new dependencies for future reference.

To ensure these new dependencies are being utilized in the project, let's run the following script:

Also applies to: 91-92

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of new logging dependencies in the project

# Test: Search for import or require statements of the new logging libraries
echo "Searching for usage of new logging libraries:"
rg --type typescript --type javascript -e "from ['\"]@logtail/pino['\"]" -e "from ['\"]pino['\"]" -e "require\(['\"]@logtail/pino['\"]" -e "require\(['\"]pino['\"]"

# Test: Check if pino-pretty is used in any npm scripts
echo "Checking for pino-pretty usage in npm scripts:"
jq '.scripts | to_entries[] | select(.value | contains("pino-pretty"))' web/package.json

Length of output: 512

Script:

#!/bin/bash
# Description: Verify the usage of new logging dependencies in the project

# Test: Search for import or require statements of the new logging libraries
echo "Searching for usage of new logging libraries in .ts and .js files:"
rg -g "*.ts" -g "*.js" -e "from ['\"]@logtail/pino['\"]" -e "from ['\"]pino['\"]" -e "require\(['\"]@logtail/pino['\"]\)" -e "require\(['\"]pino['\"]\)" .

# Test: Check if pino-pretty is used in any npm scripts
echo "Checking for pino-pretty usage in npm scripts:"
jq '.scripts | to_entries[] | select(.value | contains("pino-pretty"))' web/package.json

# Additional Test: Search for pino-pretty usage in configuration files
echo "Searching for pino-pretty usage in configuration files:"
rg -g "*.config.*" "pino-pretty"

Length of output: 886

"@sentry/react": "^7.55.2",
"@sentry/tracing": "^7.55.2",
"@supabase/supabase-js": "^2.33.1",
Expand All @@ -87,6 +88,8 @@
"moment": "^2.29.4",
"overlayscrollbars": "^2.3.0",
"overlayscrollbars-react": "^0.5.2",
"pino": "^8.16.0",
"pino-pretty": "^10.2.3",
"react": "^18.2.0",
"react-chartjs-2": "^4.3.1",
"react-dom": "^18.2.0",
Expand Down
40 changes: 40 additions & 0 deletions web/src/utils/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import pino, { TransportTargetOptions } from "pino";

// Intended for Netlify functions
export const createLogger = (logtailToken?: string): pino.Logger => {
const targets: TransportTargetOptions[] = [
{
target: "pino-pretty",
options: {},
level: "info",
},
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider removing pino-pretty from production logging

pino-pretty is intended for development and debugging purposes and is not recommended for use in production environments due to performance overhead and potential security concerns. Including it in your production logger could impact performance.

];
if (logtailToken) {
targets.push({
target: "@logtail/pino",
options: { sourceToken: logtailToken },
level: "info",
});
}
return pino(
{
level: "info",
timestamp: pino.stdTimeFunctions.isoTime,
},
pino.transport({ targets: targets })
);
};

export const throwNewError = (logger: pino.Logger, message: string, error?: any) => {
if (!error) {
logger.error(message);
throw new Error(message);
}
if (typeof error === "string") {
logger.error(error, message);
throw new Error(message + ": " + error);
} else if (error instanceof Error) {
logger.error(error, message);
throw new Error(message + ": " + error.name + ": " + error.message);
}
};
Comment on lines +28 to +40
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling in throwNewError function

Consider refactoring the function to handle all error types consistently and to preserve stack traces by re-throwing the original error when possible. This ensures that the original error information is not lost, making debugging easier.

Apply this diff to improve error handling:

-export const throwNewError = (logger: pino.Logger, message: string, error?: any) => {
-  if (!error) {
-    logger.error(message);
-    throw new Error(message);
-  }
-  if (typeof error === "string") {
-    logger.error(error, message);
-    throw new Error(message + ": " + error);
-  } else if (error instanceof Error) {
-    logger.error(error, message);
-    throw new Error(message + ": " + error.name + ": " + error.message);
-  }
-};
+export const throwNewError = (logger: pino.Logger, message: string, error?: unknown): never => {
+  logger.error({ error }, message);
+  if (error instanceof Error) {
+    throw error;
+  } else if (typeof error === "string") {
+    throw new Error(`${message}: ${error}`);
+  } else {
+    throw new Error(message);
+  }
+};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const throwNewError = (logger: pino.Logger, message: string, error?: any) => {
if (!error) {
logger.error(message);
throw new Error(message);
}
if (typeof error === "string") {
logger.error(error, message);
throw new Error(message + ": " + error);
} else if (error instanceof Error) {
logger.error(error, message);
throw new Error(message + ": " + error.name + ": " + error.message);
}
};
export const throwNewError = (logger: pino.Logger, message: string, error?: unknown): never => {
logger.error({ error }, message);
if (error instanceof Error) {
throw error;
} else if (typeof error === "string") {
throw new Error(`${message}: ${error}`);
} else {
throw new Error(message);
}
};

123 changes: 123 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5365,6 +5365,7 @@ __metadata:
"@kleros/kleros-v2-prettier-config": "workspace:^"
"@kleros/kleros-v2-tsconfig": "workspace:^"
"@kleros/ui-components-library": ^2.6.2
"@logtail/pino": ^0.4.12
"@netlify/functions": ^1.6.0
"@parcel/transformer-svg-react": 2.8.3
"@parcel/watcher": ~2.2.0
Expand Down Expand Up @@ -5401,6 +5402,8 @@ __metadata:
overlayscrollbars: ^2.3.0
overlayscrollbars-react: ^0.5.2
parcel: 2.8.3
pino: ^8.16.0
pino-pretty: ^10.2.3
react: ^18.2.0
react-chartjs-2: ^4.3.1
react-dom: ^18.2.0
Expand Down Expand Up @@ -5663,6 +5666,16 @@ __metadata:
languageName: node
linkType: hard

"@logtail/core@npm:^0.4.12":
version: 0.4.12
resolution: "@logtail/core@npm:0.4.12"
dependencies:
"@logtail/tools": ^0.4.12
"@logtail/types": ^0.4.11
checksum: 615874d456ed732d650d4a022bd14e2a400198b26dfd07cb512a365e8369a8ab1d48b74657c7bc1a77e03b430778f652d41c84638c3efe9de2ef91cfe6f4dd29
languageName: node
linkType: hard

"@logtail/node@npm:^0.4.0":
version: 0.4.0
resolution: "@logtail/node@npm:0.4.0"
Expand All @@ -5678,6 +5691,21 @@ __metadata:
languageName: node
linkType: hard

"@logtail/node@npm:^0.4.12":
version: 0.4.12
resolution: "@logtail/node@npm:0.4.12"
dependencies:
"@logtail/core": ^0.4.12
"@logtail/types": ^0.4.11
"@msgpack/msgpack": ^2.5.1
"@types/stack-trace": ^0.0.29
cross-fetch: ^3.0.4
minimatch: ^3.0.4
stack-trace: ^0.0.10
checksum: d2799eb561e4bdee4fb9875350983cb5f7ede2ab6c368bf3673cbcfdc7df3c16616d6b881edf60dfe64e0bafc6c6a63d96455bb0a4d34328ed3414fd94d47c5f
languageName: node
linkType: hard

"@logtail/pino@npm:^0.4.0":
version: 0.4.0
resolution: "@logtail/pino@npm:0.4.0"
Expand All @@ -5691,6 +5719,19 @@ __metadata:
languageName: node
linkType: hard

"@logtail/pino@npm:^0.4.12":
version: 0.4.12
resolution: "@logtail/pino@npm:0.4.12"
dependencies:
"@logtail/node": ^0.4.12
"@logtail/types": ^0.4.11
pino-abstract-transport: ^1.0.0
peerDependencies:
pino: ^7.0.0 || ^8.0.0
checksum: 1b6ee658ad5e60c65cc0bd50aaf1e99b26150e2b26622a13c09fcb4e15f33dd8979aa58401d06524d82a5ea19511c96871bbdb57aaa4166586dc76159b01110e
languageName: node
linkType: hard

"@logtail/tools@npm:^0.4.0":
version: 0.4.0
resolution: "@logtail/tools@npm:0.4.0"
Expand All @@ -5700,6 +5741,15 @@ __metadata:
languageName: node
linkType: hard

"@logtail/tools@npm:^0.4.12":
version: 0.4.12
resolution: "@logtail/tools@npm:0.4.12"
dependencies:
"@logtail/types": ^0.4.11
checksum: b0a8391a763b7f7f7d889446ed857715f158288f615b089f125471582efd8bb2a9525e37bb58f88f166224529ce3bc74799807ff768905e79ac1c92d5877aa1e
languageName: node
linkType: hard

"@logtail/types@npm:^0.4.0":
version: 0.4.0
resolution: "@logtail/types@npm:0.4.0"
Expand All @@ -5709,6 +5759,15 @@ __metadata:
languageName: node
linkType: hard

"@logtail/types@npm:^0.4.11":
version: 0.4.11
resolution: "@logtail/types@npm:0.4.11"
dependencies:
js: ^0.1.0
checksum: 0102a079d0ba89efc70770f6d853f0b205784a73abe570ab056f830d4ab031f95a1d80e2a3dfa54ebf53193d20fd9b61d52c2be03032d28572ce9fcb3be89144
languageName: node
linkType: hard

"@metamask/eth-sig-util@npm:^4.0.0":
version: 4.0.1
resolution: "@metamask/eth-sig-util@npm:4.0.1"
Expand Down Expand Up @@ -24731,6 +24790,16 @@ __metadata:
languageName: node
linkType: hard

"pino-abstract-transport@npm:v1.1.0":
version: 1.1.0
resolution: "pino-abstract-transport@npm:1.1.0"
dependencies:
readable-stream: ^4.0.0
split2: ^4.0.0
checksum: cc84caabee5647b5753ae484d5f63a1bca0f6e1791845e2db2b6d830a561c2b5dd1177720f68d78994c8a93aecc69f2729e6ac2bc871a1bf5bb4b0ec17210668
languageName: node
linkType: hard

"pino-pretty@npm:^10.0.0":
version: 10.0.0
resolution: "pino-pretty@npm:10.0.0"
Expand All @@ -24755,6 +24824,30 @@ __metadata:
languageName: node
linkType: hard

"pino-pretty@npm:^10.2.3":
version: 10.2.3
resolution: "pino-pretty@npm:10.2.3"
dependencies:
colorette: ^2.0.7
dateformat: ^4.6.3
fast-copy: ^3.0.0
fast-safe-stringify: ^2.1.1
help-me: ^4.0.1
joycon: ^3.1.1
minimist: ^1.2.6
on-exit-leak-free: ^2.1.0
pino-abstract-transport: ^1.0.0
pump: ^3.0.0
readable-stream: ^4.0.0
secure-json-parse: ^2.4.0
sonic-boom: ^3.0.0
strip-json-comments: ^3.1.1
bin:
pino-pretty: bin.js
checksum: 9182886855515000df2ef381762c69fc29dbdd9014a76839cc3d8a7a94ac96d4ce17423adb9ddd61eae78986bb0ff3a1d9e6e7aa55476c096a3dd4a0c89440e8
languageName: node
linkType: hard

"pino-std-serializers@npm:^4.0.0":
version: 4.0.0
resolution: "pino-std-serializers@npm:4.0.0"
Expand Down Expand Up @@ -24811,6 +24904,27 @@ __metadata:
languageName: node
linkType: hard

"pino@npm:^8.16.0":
version: 8.16.0
resolution: "pino@npm:8.16.0"
dependencies:
atomic-sleep: ^1.0.0
fast-redact: ^3.1.1
on-exit-leak-free: ^2.1.0
pino-abstract-transport: v1.1.0
pino-std-serializers: ^6.0.0
process-warning: ^2.0.0
quick-format-unescaped: ^4.0.3
real-require: ^0.2.0
safe-stable-stringify: ^2.3.1
sonic-boom: ^3.7.0
thread-stream: ^2.0.0
bin:
pino: bin.js
checksum: c3af0d1d80d0a7ec59530e6c3668895ac813762829ea0b7e316057370f58011d09e128e67289665652904367a1f27f87cca4e564eb3ff2a0d46219f12fcf896e
languageName: node
linkType: hard

"pirates@npm:^4.0.1, pirates@npm:^4.0.4":
version: 4.0.6
resolution: "pirates@npm:4.0.6"
Expand Down Expand Up @@ -28387,6 +28501,15 @@ __metadata:
languageName: node
linkType: hard

"sonic-boom@npm:^3.7.0":
version: 3.7.0
resolution: "sonic-boom@npm:3.7.0"
dependencies:
atomic-sleep: ^1.0.0
checksum: 528f0f7f7e09dcdb02ad5985039f66554266cbd8813f9920781607c9248e01f468598c1334eab2cc740c016a63c8b2a20e15c3f618cddb08ea1cfb4a390a796e
languageName: node
linkType: hard

"source-list-map@npm:^2.0.0, source-list-map@npm:^2.0.1":
version: 2.0.1
resolution: "source-list-map@npm:2.0.1"
Expand Down