@woop Does the current E2E auth tests verify that there are no false positives for authentication? (ie should fail to authenticate due to wrong credentials. )

#892 These tests will fail your authentication if you try to sign in with a JWT that cannot be verified.

No longer relevant as this was deemed out of scope for this PR.

Should Log4j2 be hardcoded here? #277

Used SLF4J instead.

Shouldn't the format be a part of the logger configuration xml instead of a part of the code base?

The idea was to expose the ability to configure the logger with the most common use cases via application.yml without interacting with the logging configuration XML as it inaccessible from our Helm and Docker-Compose setup.

As mentioned in the response above, I think we can use the application.yml for configuring logging without writing our own logformat configuration.

Removed.

I cant make out this comment. What does the direction represent?

Direction represents the flow of messages relative to the service:

• incoming - matches requests messages
• outgoing - matches response messages.

Users can use this to configure for which request "direction" message audit logging is enabled. (ie leave in only incoming to enable request logging)

Will change this to request and response as that makes it easier to understand.

Should this be in a separate PR?

Yeah, will do.

Does this work for you? I have been pulling the identity from claims.

Yes.

Is this supposed to apply to GetOnlineFeatures as well, because in that case we will probably experience a serious performance hit. Audit logging is normally distinct from request/response logging, since with the former you expect durability and consistency but with the latter you want availability and performance. Logging the complete request/response is still useful as part of the audit log, but we need to have a way to deal with the latency sensitive case.

The AuditLogger can be configured via application.yml via AuditLogProperties and thus can be disabled for the Online Serving instance from application.yml for cases where latency is more important that visibility.

Yep, but we need request/response logging in online serving as well. We just want to make sure that this logging happens in async fashion and doesn't affect latency of the method

My initial reaction when seeing this code is that it seems quite overwhelming. A lot of decisions have been made here in terms of the structure of our audit logging and I am not sure if we are being too ambitious here. I am quite worried that these abstractions will end up changing quite soon once people start trying to use the audit logger. I was hoping we could start with a simpler key/value API.

Did you follow some kind of convention when structuring the audit logger and its associated types, or was everything custom made?

Did you follow some kind of convention when structuring the audit logger and its associated types, or was everything custom made?

No, Yes.

I was hoping we could start with a simpler key/value API.

Reasons that we should go with a Key Value API.

• Infinitely more flexible and extensible as no constraints are applied.

Reasons that we should not go with a Key Value API:

• Java is statically typed. Going with a generic Key Value would mean Map<String, Object> which implies lots of ugly casting.
• With no enforced structure in place it will be difficult to parse the logs in upstream systems as there is no expected structure.
• Enforces some rigidity to logging so that we don't break upstream systems unknowing built upon audit logging.

The structured log will be depended on by upstream logging systems and should be treated as a user facing API that should be clearly defined, the same way Core and Serving Service API are clearly defined.

I am quite worried that these abstractions will end up changing quite soon once people start trying to use the audit logger

The important thing is that we are fully aware of changes instead of being blind sighted by a map.put()

My suggestion is not to throw away all static typing and just accept arbitrary string/object keys and values. Parts of what you have implemented makes sense, but other parts do not.

From a functional perspective what we want is for specific parts of the Feast code base to create a message that can be serialized and printed/sent to the audit log. so having static methods like ofMessage(MessageLogEvent.Kind kind, String method, Message message, String identity) makes sense.

However, I question the value of this LogEvent as a container. It seems like a catchall class that provides little in terms of a contract. When would you have both a MessageLogEvent and ActionLogEvent in the same object? It doesnt seem like that should be possible and so its not clear what the scope of this LogEvent class should be. Will it just grow infinitely as a catchall class that can contain any subclass?

If we agree that maintaining state is a bad thing, then do we need to maintain LogEvents as an object? What is the SLF4J contract that we need to meet. Does it just expect strings?

What prevents us from having an AuditLogger class with static methods like ofMessage that converts a tight contract of objects into a Map or string that can be serialized?

Refactored away LogEvent into MessageAuditLogEntry, ActionAuditLogEntry, TransitionAuditLogEntry subclasses.

What is this testing? Wouldnt toString() always return something?

Yeah, I guess I was just trying to exercise the code path. Would remove.

This code looks much better with the exception handling in the interceptor, but I don't think its in scope for this PR is it?

Yeah, moving out of this PR.

Why did we add this constructor?

Originally this used the @Builder constructor from lombok and had fields duplicated across all 4 Job Task implementations. I move the fields up to JobTask abstract class however the @builder annotation has problems with inherited fields, hence the added constructor.

It bothers me a bit that we are pulling in logging dependencies directly into our code. Shouldn't this be abstracted away?

Replaced with SL4J.

Is it possible for us to keep things simpler by using SLF4J for logging instead of having our own AuditLogger? Example: https://github.com/yidongnan/grpc-spring-boot-starter/blob/master/examples/local-grpc-server/src/main/java/net/devh/boot/grpc/examples/local/server/LogGrpcInterceptor.java#L34

I can see some value in having a way to standardize the message format through static methods, especially if we need to have actions/resources/identities, but having to pass around an audit logger when SLF4J is already available seems like an unnecessary overhead to me.

Is it possible for us to keep things simpler by using SLF4J for logging instead of having our own AuditLogger? Example: https://github.com/yidongnan/grpc-spring-boot-starter/blob/master/examples/local-grpc-server/src/main/java/net/devh/boot/grpc/examples/local/server/LogGrpcInterceptor.java#L34

Why we have our own Audit Logger:

• Allow us to configure the audit logger from application.yml which is valuable as:
  • Allows us to configure the logger via application.yml (ie disable request logging for Online Serving without hard coding something.)
  • We can inject Feast specific elements into the Audit Log (ie Feast Component, Feast Version, or in the future git hash.)

Allows us to configure the logger via application.yml (ie disable request logging for Online Serving without hard coding something.)

What have we implemented that can't be implemented using the normal logging configuration https://howtodoinjava.com/spring-boot2/logging/configure-logging-application-yml/

Also, it seems like your answer focuses on configuration but the AuditLogger is more than just a configuration holder right? We can have custom configuration in the FeastProperties for enabling/disabling request/response logging if needed, but that is a separate question to whether we need to maintain the AuditLogger and pass it around.

We can inject Feast specific elements into the Audit Log

Since we have an application context just floating around inside of Feast, we can easily grab these settings at any time without needing to pass around an AuditLogger right? I am just trying to make sure we arent reinventing something that already exists.

Updated AuditLogger's logging methods to static to remove the need of passing around the AuditLogger instance.

Do we really need to set this? Shouldnt this be set in the parent-pom?

This was set as the interceptors do not work properly under the lognet grpc starter.

Removed version part to be set in the parent POM.

Why was this changed?

Reverted.

Can we call this something more descriptive? enableMessageLogging?

Ok. updated.

Still not sure why we need to persist with FeastInstance.