Conversation
e3e142a to
161d4d3
Compare
ba4fb8d to
c745dbc
Compare
76ff504 to
65b16a8
Compare
…rformance chore: add needed IDX
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an OpenTelemetry (OTLP) audit strategy for logging entity changes as telemetry data instead of traditional database records. The implementation provides a new audit mechanism that can send audit logs to external observability systems like Elasticsearch through OpenTelemetry collectors.
- Implements OTLP-based audit strategy using OpenTelemetry logging
- Adds comprehensive test coverage for the new audit functionality
- Configures OpenTelemetry collector to export logs to Elasticsearch
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/Audit/Interfaces/IAuditStrategy.php | Defines contract for audit strategy implementations |
| app/Audit/AuditLogOtlpStrategy.php | Main OTLP audit strategy implementation with telemetry logging |
| app/Audit/AuditEventListener.php | Updated to support strategy selection between DB and OTLP auditing |
| tests/OpenTelemetry/AuditOtlpStrategyTest.php | Comprehensive tests for OTLP audit strategy with real entities |
| tests/OpenTelemetry/AuditEventTypesTest.php | Tests for different audit event types (create, update, delete) |
| tests/OpenTelemetry/OpenTelemetryTestCase.php | Adds OTEL_LOGS_EXPORTER environment variable for test isolation |
| docker-compose/opentelemetry/otel-collector-config.yaml | Configures Elasticsearch as logs exporter |
| .env.example | Adds configuration for Elasticsearch index name |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/Audit/AuditLogOtlpStrategy.php
Outdated
| $data['id'] = $entity->getId(); | ||
| } | ||
|
|
||
| if (method_exists($entity, 'getTitle')) { | ||
| $data['title'] = $entity->getTitle(); | ||
| } | ||
|
|
||
| if (method_exists($entity, 'getName')) { | ||
| $data['name'] = $entity->getName(); | ||
| } | ||
|
|
||
| if (method_exists($entity, 'getSlug')) { | ||
| $data['slug'] = $entity->getSlug(); |
There was a problem hiding this comment.
Consider adding null checks for method return values to prevent null values in audit data, which could cause issues downstream in log processing systems.
| $data['id'] = $entity->getId(); | |
| } | |
| if (method_exists($entity, 'getTitle')) { | |
| $data['title'] = $entity->getTitle(); | |
| } | |
| if (method_exists($entity, 'getName')) { | |
| $data['name'] = $entity->getName(); | |
| } | |
| if (method_exists($entity, 'getSlug')) { | |
| $data['slug'] = $entity->getSlug(); | |
| $id = $entity->getId(); | |
| if ($id !== null) { | |
| $data['id'] = $id; | |
| } | |
| } | |
| if (method_exists($entity, 'getTitle')) { | |
| $title = $entity->getTitle(); | |
| if ($title !== null) { | |
| $data['title'] = $title; | |
| } | |
| } | |
| if (method_exists($entity, 'getName')) { | |
| $name = $entity->getName(); | |
| if ($name !== null) { | |
| $data['name'] = $name; | |
| } | |
| } | |
| if (method_exists($entity, 'getSlug')) { | |
| $slug = $entity->getSlug(); | |
| if ($slug !== null) { | |
| $data['slug'] = $slug; | |
| } |
| ]; | ||
| } | ||
|
|
||
| private function createSummitEventChangeSet($summitEvent): array |
There was a problem hiding this comment.
Parameter should have a type hint for better type safety and IDE support. Consider adding the specific entity type or a base interface.
| private function createSummitEventChangeSet($summitEvent): array | |
| private function createSummitEventChangeSet(object $summitEvent): array |
smarcet
left a comment
There was a problem hiding this comment.
@andrestejerina97 please review comments
app/Audit/AuditEventListener.php
Outdated
| foreach ($uow->getScheduledCollectionUpdates() as $col) { | ||
| $strategy->audit($col, null, $strategy::EVENT_COLLECTION_UPDATE); | ||
| // Check if OTLP audit is enabled | ||
| if (env('OTEL_SERVICE_ENABLED', false)) { |
There was a problem hiding this comment.
please review this logic
if (!env('OTEL_SERVICE_ENABLED', false)) return new AuditLogStrategy($em);
try {
return App::make(AuditLogOtlpStrategy::class);
} catch (\Exception $e) {
Log::warning('Failed to create OTLP audit strategy, falling back to database', [
'error' => $e->getMessage()
]);
// Fall through to database strategy
}
return null
There was a problem hiding this comment.
I adjusted the logic to use OTLP, and if that fails, it uses AuditLog DB
smarcet
left a comment
There was a problem hiding this comment.
@andrestejerina97 please review comments
d982d42 to
06ab148
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
app/Audit/AuditLogOtlpStrategy.php
Outdated
| $this->enabled = env('OTEL_SERVICE_ENABLED', false); | ||
|
|
||
| $this->elasticIndex = env('OTEL_AUDIT_ELASTICSEARCH_INDEX', 'logs-audit'); |
There was a problem hiding this comment.
The env() function should not be used directly in classes as it bypasses Laravel's configuration caching. Use config() instead and define the configuration in a config file.
| $this->enabled = env('OTEL_SERVICE_ENABLED', false); | |
| $this->elasticIndex = env('OTEL_AUDIT_ELASTICSEARCH_INDEX', 'logs-audit'); | |
| $this->enabled = config('opentelemetry.service_enabled', false); | |
| $this->elasticIndex = config('opentelemetry.audit_elasticsearch_index', 'logs-audit'); |
06ab148 to
9c4bf52
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ref: https://app.clickup.com/t/86b6r27rt