Add request response logging via fluentd - #961
Conversation
722f5cf to
21509be
Compare
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Do we really need to pull this into all modules? Can this be moved to serving only?
There was a problem hiding this comment.
AuditLogger is used in both Core and Serving, and it exists in common module, so I think that's not possible.
There was a problem hiding this comment.
Request and response should be in Json, just like the existing audit logger. Otherwise its very hard to parse in BQ.
There was a problem hiding this comment.
|
/test test-end-to-end-batch-dataflow |
6d00ace to
f85f746
Compare
|
/retest |
1 similar comment
|
/retest |
8f0ed5b to
1e6e835
Compare
7493d26 to
71ca8b0
Compare
60edf5c to
c639dca
Compare
| log.error(AUDIT_MARKER, entryJSON); | ||
| break; | ||
| // Either forward log to logging layer or log to console | ||
| if (properties.getMessageLogging().getDestination().equals(FLUENTD_DESTINATION)) { |
There was a problem hiding this comment.
What prevents this code from executing if message Logging is disabled?
There was a problem hiding this comment.
By the way, as a convention we try and initialize variables from getters in order to make code more readable. So something like
String destination = properties.getMessageLogging().getDestination();
if (destination.equals(FLUENTD_DESTINATION)) {
}
8e238a9 to
6c660c5
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terryyylim, woop The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it:
This PR introduces the option to enable request/response logging to an external fluentd service. This is an addition to the existing Audit logging capabilities which logs to the console.
Which issue(s) this PR fixes:
N/A
Does this PR introduce a user-facing change?: