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

Jump to bottom

fix: better payload for proxy logs and operations #2631

Merged
merged 4 commits into from
Aug 26, 2024
Merged

fix: better payload for proxy logs and operations #2631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72332f6
fix: better payload for proxy logs and operations
TBonnin Aug 22, 2024
897123f
fix(logs): show response body for proxy error as object if json
TBonnin Aug 22, 2024
34e8192
fix: make meta param optional in log client http method
TBonnin Aug 26, 2024
b3c6e53
fix: set retry_on_conflict when updating ES document
TBonnin Aug 26, 2024
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
17 changes: 12 additions & 5 deletions packages/logs/lib/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,24 @@ export class LogContextStateless {
});
}

async http(
message: string,
data: {
request: MessageRow['request'];
response: MessageRow['response'];
meta?: MessageRow['meta'];
}
): Promise<boolean> {
const level: MessageRow['level'] = data.response && data.response.code >= 400 ? 'error' : 'info';
return await this.log({ type: 'http', level, message, ...data, source: 'internal' });
}

/**
* @deprecated Only there for retro compat
*/
async trace(message: string, meta: MessageMeta | null = null): Promise<boolean> {
return await this.log({ type: 'log', level: 'debug', message, meta, source: 'internal' });
}

async http(message: string, data: Pick<MessageRow, 'request' | 'response' | 'meta'>): Promise<boolean> {
const level: MessageRow['level'] = data.response && data.response.code >= 400 ? 'error' : 'info';
return await this.log({ type: 'http', level, message, ...data, source: 'internal' });
}
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/logs/lib/models/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export async function update(opts: { id: OperationRow['id']; data: SetRequired<P
await client.update({
index: getFullIndexName(indexMessages.index, opts.data.createdAt),
id: opts.id,
retry_on_conflict: 3,
refresh: isTest,
body: {
doc: {
Expand Down
78 changes: 67 additions & 11 deletions packages/server/lib/controllers/proxy.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Request, Response, NextFunction } from 'express';
import type { OutgoingHttpHeaders } from 'http';
import type { OutgoingHttpHeaders, IncomingHttpHeaders } from 'http';
import type { TransformCallback } from 'stream';
import type stream from 'stream';
import { Readable, Transform, PassThrough } from 'stream';
Expand Down Expand Up @@ -161,6 +161,31 @@ class ProxyController {
}
metrics.increment(metrics.Types.PROXY_FAILURE);
next(err);
} finally {
const getHeaders = (hs: IncomingHttpHeaders | OutgoingHttpHeaders): Record<string, string> => {
const headers: Record<string, string> = {};
for (const [key, value] of Object.entries(hs)) {
if (typeof value === 'string') {
headers[key] = value;
} else if (Array.isArray(value)) {
headers[key] = value.join(', ');
}
}
return headers;
};
const reqHeaders = getHeaders(req.headers);
reqHeaders['authorization'] = 'REDACTED';
Comment on lines +165 to +177
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't use proxy.service->stripSensitiveHeaders ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it requires a proxy config but things can fail before having a valid config

await logCtx?.enrichOperation({
request: {
url: `${req.protocol}://${req.get('host')}${req.originalUrl}`,
method: req.method,
headers: reqHeaders
},
response: {
code: res.statusCode,
headers: getHeaders(res.getHeaders())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we are gonna log way more content with the headers. Is it gonna be ok with ES?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to increase the disk size anyway. It will increase the storage a bit but most logs are empty (!= operation)

}
});
}
}

Expand Down Expand Up @@ -213,7 +238,17 @@ class ProxyController {
logCtx: LogContext;
}) {
const safeHeaders = proxyService.stripSensitiveHeaders(config.headers, config);
await logCtx.info(`${config.method.toUpperCase()} ${url} was successful`, { headers: safeHeaders });
await logCtx.http(`${config.method.toUpperCase()} ${url} was successful`, {
request: {
method: config.method,
url,
headers: safeHeaders
},
response: {
code: responseStream.status,
headers: responseStream.headers as Record<string, string>
}
});

const contentType = responseStream.headers['content-type'];
const isJsonResponse = contentType && contentType.includes('application/json');
Expand Down Expand Up @@ -309,8 +344,16 @@ class ProxyController {
chunks.push(data);
});
stringify.on('end', () => {
const data = chunks.length > 0 ? Buffer.concat(chunks).toString() : 'unknown error';
void this.reportError(error, url, config, data, logCtx);
const data = chunks.length > 0 ? Buffer.concat(chunks).toString() : '';
let errorData: string | Record<string, string> = data;
if (error.response?.headers?.['content-type']?.includes('application/json')) {
try {
errorData = JSON.parse(data);
} catch {
// Intentionally left blank - errorData will be a string
}
}
void this.reportError(error, url, config, errorData, logCtx);
});
} else {
await logCtx.error('Unknown error');
Expand Down Expand Up @@ -379,14 +422,27 @@ class ProxyController {
}
}

private async reportError(error: AxiosError, url: string, config: ApplicationConstructedProxyConfiguration, errorMessage: string, logCtx: LogContext) {
private async reportError(
error: AxiosError,
url: string,
config: ApplicationConstructedProxyConfiguration,
errorContent: string | Record<string, string>,
logCtx: LogContext
) {
const safeHeaders = proxyService.stripSensitiveHeaders(config.headers, config);
await logCtx.error(`${error.request?.method.toUpperCase()} ${url} failed with status '${error.response?.status}'`, {
code: error.response?.status,
url,
error: new Error(errorMessage),
requestHeaders: safeHeaders,
responseHeaders: error.response?.headers
await logCtx.http(`${error.request?.method.toUpperCase()} ${url} failed with status '${error.response?.status}'`, {
meta: {
content: errorContent
},
request: {
method: config.method,
url,
headers: safeHeaders
},
response: {
code: error.response?.status || 500,
headers: error.response?.headers as Record<string, string>
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

note we can also modify the client to build the object automatically (like the way I did with error) just to reuse the logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure it make sense to tie logs to Axios.

});
await logCtx.failed();
}
Expand Down
11 changes: 7 additions & 4 deletions packages/webapp/src/pages/Logs/ShowMessage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ export const ShowMessage: React.FC<{ message: MessageRow }> = ({ message }) => {
}, [message.createdAt]);

const payload = useMemo(() => {
if (!message.meta && !message.error) {
if (!message.meta && !message.error && !message.request && !message.response) {
return null;
}

const pl: Record<string, any> = {};
if (message.meta) {
pl.output = message.meta;
const pl: Record<string, any> = message.meta ? { ...message.meta } : {};
if (message.request) {
pl.request = message.request;
}
if (message.response) {
pl.response = message.response;
}
if (message.error) {
pl.error = message.error.message;
Expand Down
19 changes: 17 additions & 2 deletions packages/webapp/src/pages/Logs/ShowOperation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,21 @@ export const ShowOperation: React.FC<{ operationId: string }> = ({ operationId }
return !operation || operation.state === 'waiting' || operation.state === 'running';
}, [operation]);

const payload = useMemo(() => {
if (!operation?.meta && !operation?.request && !operation?.response) {
return null;
}

const pl: Record<string, any> = operation.meta ? { ...operation.meta } : {};
if (operation.request) {
pl.request = operation.request;
}
if (operation.response) {
pl.response = operation.response;
}
return pl;
}, [operation?.meta, operation?.request, operation?.response]);

useInterval(
() => {
// Auto refresh
Expand Down Expand Up @@ -154,7 +169,7 @@ export const ShowOperation: React.FC<{ operationId: string }> = ({ operationId }
</div>
<div className="">
<h4 className="font-semibold text-sm mb-2">Payload</h4>
{operation.meta ? (
{payload ? (
<div className="text-gray-400 text-sm bg-pure-black py-2 max-h-36 overflow-y-scroll">
<Prism
language="json"
Expand All @@ -164,7 +179,7 @@ export const ShowOperation: React.FC<{ operationId: string }> = ({ operationId }
return { code: { padding: '0', whiteSpace: 'pre-wrap' } };
}}
>
{JSON.stringify(operation.meta, null, 2)}
{JSON.stringify(payload, null, 2)}
</Prism>
</div>
) : (
Expand Down
Loading