With the default auditor service simply forwarding to the logger service I don't think we need a concrete mock implementation and can use the default implementation instead

I believe the auditor mock is still valuable to retain, as its implementation details don't exactly mirror those of the rootLogger. Let me know if I’m misunderstanding your point.

It looks pretty much identical to me, except that it doesn't have support for the Winston transport ofc but it's not worth duplicating the code to remove that. I think it should be fine to leave this one without explicit mocks, similar to this and the ones below:

I realize we likey want a way to supply credentials separately from a request, or essentially provide the equivalent fields of what we'd gather from the request manually. Thinking especially for situations when we use other protocols than plain HTTP. Happy to leave that as a followup though.

When this PR was initially opened, the implementation included a way to provide credentials directly. However, that approach was updated to fallback onto service that triggered it. For reference, see. Although, let's discuss this more in a follow up.

Why this indirection? Could just implement log internally in DefaultAuditorService instead right?

I'm thinking it's worth having DefaultAuditorService just take a log callback or something like that, receiving a message and meta, as in same signature as .info(...) but slimmed down to exactly what we need. That way it's actually kind of simple to reimplement this service with any kind of log function.

You're raising a valid point about simplifying the DefaultAuditorService by directly implementing the log method within it and accepting a callback. While seemingly straightforward, this approach would sacrifice crucial functionalities related to context and structured logging.

Currently, the DefaultRootAuditorService isn't just a wrapper around a Winston logger. It plays a vital role in managing the overall logging context, including consistent formatting, setting the service name, and propagating that context to individual plugin instances. This ensures uniformity across all audit logs, simplifying filtering and analysis.

By creating DefaultAuditorService instances via forPlugin, each plugin inherits the root context and can append its own specific details. This hierarchy makes it easy to track events originating from different plugins while maintaining a standardized format. A simple callback wouldn't allow for such structured context propagation. Consider a scenario where we want to filter logs by plugin or include common fields like service name in every log entry. The current design facilitates these use cases without requiring each plugin to re-implement the same logic.

Moreover, allowing plugins to completely customize logging via a callback could lead to inconsistencies and make analysis across plugins difficult. The present structure strikes a balance between flexibility for plugins (through metadata and events) and enforced consistency for the overall system. Therefore, while a callback approach appears simpler, it doesn't address the broader needs of a structured, consistent, and context-aware audit logging system.

Hmm, doesn't feel quite right to default to service credentials here. I think it would be better to leave as undefined tbh. We kinda hide information by doing this, as in we don't know whether these credentials where explicitly passed or if we hit this fallback.

I understand the concern, but tracking the actor—even if it’s the fallback—is important because auditors typically require an actor in audit logs. Having more detailed information allows us to trace where an event originated. For example, in the Scaffolder plugin, there are multiple scenarios where credentials (request objects) aren’t explicitly provided, and using the fallback helps us identify the service responsible. Leaving it undefined might result in gaps in the audit trail, which could create challenges when backtracking events.

Thinking the default message should probably contain a lil bit more information than just the even ID right? Even if it's just something like Audit event ${eventId} ${status}

I’d prefer to align with Google's format, where the event ID acts as a reference. Additionally, there’s an isAuditorEvent field to clearly distinguish between standard logs and auditor-specific logs.

Relevant implementation: https://github.com/schultzp2020/backstage/blob/event-auditor/packages/backend-defaults/src/entrypoints/auditor/Auditor.ts#L282-L284

It looks pretty much identical to me, except that it doesn't have support for the Winston transport ofc but it's not worth duplicating the code to remove that. I think it should be fine to leave this one without explicit mocks, similar to this and the ones below: