-
Notifications
You must be signed in to change notification settings - Fork 56
Add a GUC to log HTTP traffic #71
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
Conversation
sfc-gh-mslot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the GUC name I was sort of expecting this would LOG the requests
|
|
||
| if (EnableHttpClientLogging) | ||
| { | ||
| ereport(NOTICE, (errmsg("preparing HTTP request to URL: %s\nRequest Type: %s \nRequest Body: %s", url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, would have a slight preference for single line where possible and skipping the N/A
making GET request to URL https://...
making POST request URL https://.. : {...}
I think log would be too much?
I guess I changed the implementation a bit, after reading through your review. I changed it to also show the responses from the server. It is actually more interesting to have the full traffic, so the name reflects that as well |
9900d26 to
b5404f6
Compare
b5404f6 to
2a9c1a9
Compare
|
We might want to redact access token in the log if it hits into the server log as well. |
| appendStringInfo(postDataInfo, " : {%s}", postData); | ||
| } | ||
|
|
||
| ereport(INFO, (errmsg("making %s request to URL %s%s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ereport(INFO, (errmsg("making %s request to URL %s%s", | |
| ereport(INFO, (errmsg("making %s request to URL %s, body: %s", |
| #include <string.h> | ||
| #include <ctype.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move top
|
|
||
|
|
||
| DefineCustomBoolVariable( | ||
| "pg_lake_iceberg.http_client_trace_traffic", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Does pg_lake_iceberg.http_client_debug sound clear?
Especially useful for #68
a4096d8 to
7dae279
Compare
Especially useful for #68
An example from #68: