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.

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

fix: better payload for proxy logs and operations #2631

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

TBonnin
Copy link
Collaborator

@TBonnin TBonnin commented Aug 22, 2024

Showing request and response info in operations and logs payload for proxy call.

Operation payload shows info about the request to nango api
Log payload shows info about the request to the 3rd party api

Successful operation:
Screenshot 2024-08-22 at 13 07 53
Successful log:
Screenshot 2024-08-22 at 13 08 01

Failed operation:
Screenshot 2024-08-22 at 13 07 34
Failed log:
Screenshot 2024-08-22 at 13 07 43

Issue ticket number and link

https://linear.app/nango/issue/NAN-1526/improve-proxy-logs

@@ -213,7 +238,18 @@ 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`, {
meta: null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

meta could potentially be optional. Not a blocker for now though

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 definitely removed it from the required field

},
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)

Copy link
Collaborator

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

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

Works great, few comments.
However it doesn't show request/response on success and I don't know why looking at the code 🤔

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.

Comment on lines +165 to +177
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';
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

@TBonnin
Copy link
Collaborator Author

TBonnin commented Aug 26, 2024

Works great, few comments. However it doesn't show request/response on success and I don't know why looking at the code 🤔

I had the following error once after switching branches

...
Root causes:\n\t\tversion_conflict_engine_exception: [K6FDcMae6BnUbxqxn4ga]: version conflict, required seqNo [5], primary term [1]. current document has seqNo [6] and primary term [1]"}
[orc] 09:09:30.743 info

Did you get the same thing?

@TBonnin TBonnin force-pushed the tbonnin/proxy-logs-improvements branch from 046253a to a50da8d Compare August 26, 2024 14:37
@TBonnin TBonnin force-pushed the tbonnin/proxy-logs-improvements branch from a50da8d to b3c6e53 Compare August 26, 2024 15:13
@TBonnin TBonnin merged commit c85f9c8 into master Aug 26, 2024
27 checks passed
@TBonnin TBonnin deleted the tbonnin/proxy-logs-improvements branch August 26, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants