Skip to content

[core.logging] Fix issue where logs fail to calculate size of gunzip streams. #90353

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

Merged
merged 1 commit into from
Feb 5, 2021
Merged

[core.logging] Fix issue where logs fail to calculate size of gunzip streams. #90353

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
11 changes: 11 additions & 0 deletions packages/kbn-legacy-logging/src/utils/get_payload_size.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Side Public License, v 1.
*/

import { createGunzip } from 'zlib';
import mockFs from 'mock-fs';
import { createReadStream } from 'fs';

Expand Down Expand Up @@ -54,6 +55,11 @@ describe('getPayloadSize', () => {
const result = getResponsePayloadBytes(readStream);
expect(result).toBe(Buffer.byteLength(data));
});

test('ignores streams that are not instances of ReadStream', async () => {
const result = getResponsePayloadBytes(createGunzip());
expect(result).toBe(undefined);
});
});

describe('handles plain responses', () => {
Expand All @@ -72,6 +78,11 @@ describe('getPayloadSize', () => {
const result = getResponsePayloadBytes(payload);
expect(result).toBe(JSON.stringify(payload).length);
});

test('returns undefined when source is not plain object', () => {
const result = getResponsePayloadBytes([1, 2, 3]);
Copy link
Contributor

@mshustov mshustov Feb 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it seems to be a valid use-case when you want to respond with an array of objects. isn't it? @lukeelmers

expect(result).toBe(undefined);
});
});

describe('handles content-length header', () => {
Expand Down
9 changes: 4 additions & 5 deletions packages/kbn-legacy-logging/src/utils/get_payload_size.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@
* Side Public License, v 1.
*/

import type { ReadStream } from 'fs';
import { isPlainObject } from 'lodash';
import { ReadStream } from 'fs';
import type { ResponseObject } from '@hapi/hapi';

const isBuffer = (obj: unknown): obj is Buffer => Buffer.isBuffer(obj);
const isObject = (obj: unknown): obj is Record<string, unknown> =>
typeof obj === 'object' && obj !== null;
const isFsReadStream = (obj: unknown): obj is ReadStream =>
typeof obj === 'object' && obj !== null && 'bytesRead' in obj;
typeof obj === 'object' && obj !== null && 'bytesRead' in obj && obj instanceof ReadStream;
const isString = (obj: unknown): obj is string => typeof obj === 'string';

/**
Expand Down Expand Up @@ -56,7 +55,7 @@ export function getResponsePayloadBytes(
return Buffer.byteLength(payload);
}

if (isObject(payload)) {
if (isPlainObject(payload)) {
return Buffer.byteLength(JSON.stringify(payload));
}

Expand Down
24 changes: 24 additions & 0 deletions src/core/server/http/logging/get_payload_size.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* Side Public License, v 1.
*/

import { createGunzip } from 'zlib';
import type { Request } from '@hapi/hapi';
import Boom from '@hapi/boom';

Expand Down Expand Up @@ -96,6 +97,18 @@ describe('getPayloadSize', () => {

expect(result).toBe(Buffer.byteLength(data));
});

test('ignores streams that are not instances of ReadStream', async () => {
const result = getResponsePayloadBytes(
{
variety: 'stream',
source: createGunzip(),
} as Response,
logger
);

expect(result).toBe(undefined);
});
});

describe('handles plain responses', () => {
Expand Down Expand Up @@ -132,6 +145,17 @@ describe('getPayloadSize', () => {
);
expect(result).toBe(JSON.stringify(payload).length);
});

test('returns undefined when source is not a plain object', () => {
const result = getResponsePayloadBytes(
{
variety: 'plain',
source: [1, 2, 3],
} as Response,
logger
);
expect(result).toBe(undefined);
});
});

describe('handles content-length header', () => {
Expand Down
22 changes: 16 additions & 6 deletions src/core/server/http/logging/get_payload_size.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
* Side Public License, v 1.
*/

import type { ReadStream } from 'fs';
import { isPlainObject } from 'lodash';
import { ReadStream } from 'fs';
import { isBoom } from '@hapi/boom';
import type { Request } from '@hapi/hapi';
import { Logger } from '../../logging';
Expand All @@ -17,8 +18,15 @@ const isBuffer = (src: unknown, res: Response): src is Buffer => {
return !isBoom(res) && res.variety === 'buffer' && res.source === src;
};
const isFsReadStream = (src: unknown, res: Response): src is ReadStream => {
return !isBoom(res) && res.variety === 'stream' && res.source === src;
return (
!isBoom(res) &&
res.variety === 'stream' &&
res.source === src &&
res.source instanceof ReadStream
Copy link
Contributor

@TinaHeiligers TinaHeiligers Feb 4, 2021

Choose a reason for hiding this comment

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

suggestion but not a blocker: add an inline comment as to why we need to explicitly check the instance type:

Suggested change
res.source instanceof ReadStream
res.source instanceof ReadStream // prevent zlib's Gunzip streams through

);
};
const isString = (src: unknown, res: Response): src is string =>
!isBoom(res) && res.variety === 'plain' && typeof src === 'string';

/**
* Attempts to determine the size (in bytes) of a Hapi response
Expand Down Expand Up @@ -57,10 +65,12 @@ export function getResponsePayloadBytes(response: Response, log: Logger): number
return response.source.bytesRead;
}

if (response.variety === 'plain') {
return typeof response.source === 'string'
? Buffer.byteLength(response.source)
: Buffer.byteLength(JSON.stringify(response.source));
if (isString(response.source, response)) {
return Buffer.byteLength(response.source);
}

if (response.variety === 'plain' && isPlainObject(response.source)) {
return Buffer.byteLength(JSON.stringify(response.source));
}
} catch (e) {
// We intentionally swallow any errors as this information is
Expand Down