Skip to content

Commit

Permalink
Only send 2 MiB of log data in diagnose report (#415)
Browse files Browse the repository at this point in the history
Don't send too much log file data. We previously decided upon 2 MiB of
data for the appsignal.log in the Ruby gem, so apply that in the Node.js
integration too.

I wrote the reading of this data in a sync way, with `Sync` functions
from the `fs` module. I could not get it to work with promises and async
without rewriting the entire script to be async, which seemed like a lot
of work for this change.

This sync approach is less safe I think, because we need to close the
open file manually. To make it more safe I've added the file closing to
the `finally` statement that triggers in the try-block regardless if an
error occurs in the try-block. The finally-block is like an ensure-block
in Ruby.

Closes #413
  • Loading branch information
tombruijn authored Jul 16, 2021
1 parent 24693ed commit baa2b7f
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
bump: "patch"
---

Limit the `appsignal.log` file size in diagnose report. It will only send the last 2MiB of the file. This prevents the diagnose report from sending too much data that it gets rejected by the server.
28 changes: 24 additions & 4 deletions packages/nodejs/src/__tests__/diagnose.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import fs from "fs"
import { DiagnoseTool } from "../diagnose"
import { VERSION, AGENT_VERSION } from "../version"

Expand Down Expand Up @@ -31,10 +32,29 @@ describe("DiagnoseTool", () => {
expect(tool.generate().paths.log_dir_path.path).toEqual("/tmp")
})

it("returns the appsignal.log path", () => {
expect(tool.generate().paths["appsignal.log"].path).toEqual(
"/tmp/appsignal.log"
)
describe("appsignal.log path", () => {
it("returns the appsignal.log path", () => {
expect(tool.generate().paths["appsignal.log"].path).toEqual(
"/tmp/appsignal.log"
)
})

it("returns all of the file if file is less than 2MiB in size", () => {
const fileSize = 1.9 * 1024 * 1024 // Write 1.9 MiB of content
const content = ".".repeat(fileSize)
fs.writeFileSync("/tmp/appsignal.log", content, { flag: "w" })

const contentArray = tool.generate().paths["appsignal.log"].content
expect(contentArray?.join("\n").length).toBeCloseTo(fileSize, 0)
})

it("returns maximum 2MiB of content", () => {
const content = ".".repeat(2.1 * 1024 * 1024) // Write 2.1 MiB of content
fs.writeFileSync("/tmp/appsignal.log", content, { flag: "w" })

const contentArray = tool.generate().paths["appsignal.log"].content
expect(contentArray?.join("\n").length).toEqual(2 * 1024 * 1024)
})
})

describe("when to log path is configured as a full path", () => {
Expand Down
36 changes: 35 additions & 1 deletion packages/nodejs/src/diagnose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -183,18 +183,52 @@ function getPathType(stats: fs.Stats) {
}
}

const BYTES_TO_READ_FOR_FILES = 2 * 1024 * 1024 // 2 Mebibytes

/**
* Attempts to read a UTF-8 from `path`, and either returns the result
* as a string, or an empty string on error
*/
function safeReadFromPath(path: string): string {
try {
return fs.readFileSync(path, "utf8")
return readBytesFromPath(path, BYTES_TO_READ_FOR_FILES)
} catch (_) {
return ""
}
}

function readBytesFromPath(path: string, bytesToRead: number): string {
let fd
try {
const { readLength, startPosition } = readFileOptions(path, bytesToRead)
fd = fs.openSync(path, "r")
const buffer = Buffer.alloc(readLength)
fs.readSync(fd, buffer, 0, readLength, startPosition)
return buffer.toString("utf8")
} finally {
if (fd) {
fs.closeSync(fd)
}
}
}

function readFileOptions(path: string, bytesToRead: number) {
const stats = fs.statSync(path)
const fileSize = stats.size
if (fileSize < bytesToRead) {
return {
readLength: fileSize,
startPosition: 0
}
} else {
const startPosition = fileSize - bytesToRead
return {
readLength: bytesToRead,
startPosition
}
}
}

/**
* the following lines are borrowed from https://github.com/sindresorhus/is-docker/
* thanks sindre! <3
Expand Down

0 comments on commit baa2b7f

Please sign in to comment.