Add request response logging via fluentd #961
Conversation
terryyylim
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
terryyylim
force-pushed
the
rr-logging
branch
3 times, most recently
from
722f5cf to
21509be
Compare
|
/retest |
1 similar comment
|
/retest |
terryyylim
changed the title
common/pom.xml
Outdated
| @@ -114,5 +114,10 @@ | |||
| <version>4.0</version> | |||
| <scope>test</scope> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.fluentd</groupId> | |||
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.
| fluentDLogs.put("service", entryBuilder.build().getService()); | ||
| fluentDLogs.put("status_code", entryBuilder.build().getStatusCode()); | ||
| fluentDLogs.put("method", entryBuilder.build().getMethod()); | ||
| fluentDLogs.put("request", entryBuilder.build().getRequest()); |
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.
common/src/main/java/feast/common/logging/config/LoggingProperties.java
Outdated
Show resolved
Hide resolved
|
/test test-end-to-end-batch-dataflow |
terryyylim
force-pushed
the
rr-logging
branch
5 times, most recently
from
6d00ace to
f85f746
Compare
|
/retest |
1 similar comment
|
/retest |
terryyylim
force-pushed
the
rr-logging
branch
2 times, most recently
from
8f0ed5b to
1e6e835
Compare
terryyylim
force-pushed
the
rr-logging
branch
from
7493d26 to
71ca8b0
Compare
terryyylim
force-pushed
the
rr-logging
branch
from
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
Needs 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?: