Restrict trace events to correct log level #264
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #264 +/- ##
=========================================
Coverage 84.80% 84.80%
Complexity 947 947
=========================================
Files 80 80
Lines 3798 3798
=========================================
Hits 3221 3221
Misses 577 577
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
|
Looks like a sensible change to me. Can you take a look at the phan issue? It might just need to be suppressed... |
|
Thanks Brett. Have added a phan supression - hopefully build will be 🟢 now. |
|
@alecsammon progress, now some phpstan issues. FYI you can run all of the CI checks locally: |
| $underlyingLogger = $this->logger->getLogger(); | ||
|
|
||
| /** @phan-suppress-next-line PhanUndeclaredMethod */ | ||
| if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) { |
There was a problem hiding this comment.
looks like the isHandling is only available in the monolog - my understanding is that someone could pass a custom logger.
I'm not sure I like the method_exists - but maybe is the safest way of ensuring this doesn't break any code?
There was a problem hiding this comment.
Hey Alec 👋
I had the same understanding, as it looks like PSR-3 doesn't account for that functionality.
This isn't the first method_exists() usage at least across contrib. Another option could be an instanceof check to see if the method exists and is compatible.
I wonder if OTel has some filtering mechanism in general for this, as I imagine the PSR-3 instrumentation would log everything as well.
There was a problem hiding this comment.
👋🏻 Hi!
I do think that this isn't perfect - but maybe prevents at least some leaking of debug logs into enviroments where it shouldn't be.
Looks like a processor could also be used - i.e. in a collector - to give more options for dropping events - see https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/filterprocessor/README.md.
However this is maybe more advanced - and for people with basic setups it's probably better to have this as a fallback.
There was a problem hiding this comment.
Is it expected that the underlying logger could change? If not, maybe it is possible to get the underlying one already in register and only assign to a property if method_exists passes there.
| $underlyingLogger = $this->logger->getLogger(); | ||
|
|
||
| /** @phan-suppress-next-line PhanUndeclaredMethod */ | ||
| if (method_exists($underlyingLogger, 'isHandling') && !$underlyingLogger->isHandling($log->level)) { |
There was a problem hiding this comment.
Hey Alec 👋
I had the same understanding, as it looks like PSR-3 doesn't account for that functionality.
This isn't the first method_exists() usage at least across contrib. Another option could be an instanceof check to see if the method exists and is compatible.
I wonder if OTel has some filtering mechanism in general for this, as I imagine the PSR-3 instrumentation would log everything as well.
|
@brettmc - I'm hoping that the build should all be sorted now! Are you able to re-run the workflow when you're able? |
|
Investigating build failures again - sorry :( |
|
I'm wondering if this is right approach at all. It maybe better to use a environment variable to determine which levels should be logged https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/ does describe a OTEL_LOG_LEVEL which is maybe the right one to use? Or |
|
I looked into how Java instrumentation handles this, and there for the most common logging library (see Logback appender instrumentation), the instrumented method itself is only ever called if some log level checks already passed, so this filter technically exists there even if it's due to the instrumentation hook never being reached otherwise. So there is definitely also a precedent for this approach in other OpenTelemetry implementations. As for what a global configuration option would do or if one is even needed, that could be a separate topic of discussion. If such an option were to be added, it could act merely as an additional filter, for example to exclude |
|
I'm wondering whether attaching logs as trace events is the right thing to do at all? Is there any precedent? Otel has a logging signal, and we already have contrib modules which will take monolog/psr-3 log messages and forward them in the opentelemetry format, including trace+span id. If we were to go with an environment variable, I don't think |
|
@alecsammon - are you still interested in pushing this forward? |
We noticed during testing that debug messages were appearing as events in our traces.
This change only adds events if the level would be handled by the logger - i.e. the level is at least the same as the defined minimum log level.