-
Notifications
You must be signed in to change notification settings - Fork 2
Feature/opentelemetry middleware refactor #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9f69f85 to
c349aff
Compare
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrestejerina97 please review comments
c349aff to
f5ab4d2
Compare
f5ab4d2 to
011d982
Compare
… available before logging
.env.example
Outdated
| #Open Telemetry | ||
| OTEL_SERVICE_ENABLED=true | ||
| OTEL_SERVICE_NAME=summit-api | ||
| OTEL_PROPAGATORS=tracecontext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrestejerina97 this need to add baggage too
| { | ||
| $this->logger = $logger; | ||
| $this->shouldTrack = env('APP_ENV') !== 'testing' && | ||
| config('opentelemetry.enhance_requests', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be using the same feature flag as here
Line 176 in 6e5dc66
| ], env('OTEL_SERVICE_ENABLED', false) ? [\Keepsuit\LaravelOpenTelemetry\LaravelOpenTelemetryServiceProvider::class] : []), |
| } catch (\Throwable $e) { | ||
| // forcing 'single' channel in case otlp log fails | ||
| $this->logger->channel('single')->error("Error on request tracking" . $e->getMessage()); | ||
| $this->logger->channel('single')->error("Error on request tracking: " . $e->getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this channel single logs to laravel.log we need a to change to daily
676163e to
d2b4981
Compare
baa6699 to
a1e87f3
Compare
e3e142a to
161d4d3
Compare
smarcet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ref: https://app.clickup.com/t/86b6r272w