adding otel audit mw + tests #15
Conversation
toma3233
force-pushed
the
toma/addOtelAudit
branch
from
7313d8b to
556dd3b
Compare
There was a problem hiding this comment.
PR Overview
This PR integrates OpenTelemetry audit logging into the existing HTTP logging middleware and adds corresponding tests. Key changes include:
- Updating integration tests in logging_test.go to validate audit event behavior.
- Introducing a new file logging_otelaudit.go that implements OTel audit event creation and delivery.
- Modifying the logging middleware (logging.go) to accept an OTel configuration object and trigger audit events.
Reviewed Changes
| File | Description |
|---|---|
| http/server/logging/logging_test.go | Updated tests to cover OTel audit middleware integration |
| http/server/logging/logging_otelaudit.go | Implemented functionality for creating and sending audit events |
| http/server/logging/logging.go | Modified middleware initialization to include OTel configuration |
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
http/server/logging/logging_otelaudit.go:38
- [nitpick] Consider including the remote address (req.RemoteAddr) in the error log to provide additional context for debugging the failure.
logger.Error("failed to split host and port", "error", err)
| } | ||
|
|
||
| func createOtelAuditEvent(logger slog.Logger, statusCode int, req *http.Request, otelConfig *OtelConfig) msgs.Msg { | ||
| host, _, err := net.SplitHostPort(req.RemoteAddr) |
There was a problem hiding this comment.
If net.SplitHostPort fails, the 'host' remains unset and is subsequently used by msgs.ParseAddr, which could lead to unexpected behavior. Consider handling the error with an early return or setting a safe fallback value.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Add background to the README.md file.
http/server/logging/logging.go
Outdated
| logger: *logger, | ||
| next: next, | ||
| now: time.Now, | ||
| logger: *logger, |
There was a problem hiding this comment.
It's passing *logger (a dereferenced slog.Logger) instead of logger itself when initializing the loggingMiddleware struct. Since slog.Logger is a pointer type internally, this means you are copying the entire logger, which might cause unintended behavior, especially if slog.Logger holds state.
| logger: *logger, | |
| logger: logger, |
http/server/logging/logging.go
Outdated
| logger slog.Logger | ||
| next http.Handler | ||
| now func() time.Time | ||
| logger slog.Logger |
There was a problem hiding this comment.
Accordingly, this canbe changed to pointer type.
| logger slog.Logger | |
| logger *slog.Logger |
| "github.com/microsoft/go-otel-audit/audit/msgs" | ||
| ) | ||
|
|
||
| type OtelConfig struct { |
There was a problem hiding this comment.
Looks like different naming format are being followed with struct name and filed names, let's stick to one format between PascalCase and camelCase.
There was a problem hiding this comment.
The reason why this specific struct is in pascal case is b/c OtelConfig is not an internal only struct, it needs to be available to the caller. In this case, it's not a matter of formatting but rather a Go rule that requires us to capitalize a var name to make it available outside the package.
| if err != nil { | ||
| logger.Error("failed to split host and port", "error", err) | ||
| } | ||
| addr, err := msgs.ParseAddr(host) | ||
| if err != nil { | ||
| logger.Error("failed to parse address", "error", err) | ||
| } |
There was a problem hiding this comment.
Do we still want to continue execution if encoutering an error with parsing? We can consider return early or throw an error.
There was a problem hiding this comment.
From the middleware point of view, I wanted to avoid abruptly stopping the request flow, which is why I decided to continue with execution and let the go-otel-audit package handle returning the error.
There was a problem hiding this comment.
Should we still send the message if it fails to parse? Seems like an unnecessary operation if we can't extract out a meaningful value.
There was a problem hiding this comment.
Changed it to return early and check for error in sendOtelAuditEvent function.
|
|
||
| func getOperationType(method string) msgs.OperationType { | ||
| switch method { | ||
| case http.MethodGet: |
| func getOperationType(method string) msgs.OperationType { | ||
| switch method { | ||
| case http.MethodGet: | ||
| return msgs.Read |
There was a problem hiding this comment.
Might not need this since it's same as default
Adding otel audit middleware to the existing logging http middleware
Based off otel audit framework for go: https://github.com/microsoft/go-otel-audit