Enable context-flattening to support context field searching#491
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
============================================
- Coverage 82.56% 81.35% -1.22%
+ Complexity 2306 1587 -719
============================================
Files 156 95 -61
Lines 8571 5759 -2812
============================================
- Hits 7077 4685 -2392
+ Misses 1494 1074 -420 Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
c13169b to
2e340d8
Compare
|
@brettmc I'm unclear how to trigger another CI run. I believe I've fixed all but one of the previously-failing CI tests, but I can't be sure. Is this something you need to trigger, or can I? |
2e340d8 to
ff353e9
Compare
|
Hi @brettmc, I just updated the fork by re-basing to main. Could you please run CI? thanks! |
| ->setSeverityNumber(Severity::fromPsr3($log->level)); | ||
|
|
||
| if ($this->flattenAttributes) { | ||
| foreach ($this->buildFlattenedAttributes($contextToProcess) as $key => $value) { |
There was a problem hiding this comment.
Could https://laravel.com/docs/13.x/helpers#method-array-dot work here instead? 🤔
There was a problem hiding this comment.
The implementation is much cleaner with that array method :)
1c55e20 to
42197ac
Compare
Replace the custom recursive flattenArray() method and the manual normalization loop with a single array_map() + Arr::dot() expression, removing ~25 lines of bespoke logic.
42197ac to
60ee14a
Compare
7392a9b
into
open-telemetry:main
|
Hey @nickmarden / @arthurmartelli - firstly, thanks for your contribution here. It appears that I made an error in merging this though, as it has conflicted with behaviour that changed in a later PR #511 (which had already been merged ahead of this one). This may have to be reverted to restore the previous behaviour - however, I think it is something that perhaps this PR was trying to achieve in the first instance. Is there any additional behaviour that this PR provides in addition to the changes introduced in #511 which you were still hoping to accomplish? |
|
@ChrisLightfootWild lemme take a look to see if #511 accomplishes what we need. |
|
Thanks @ChrisLightfootWild for bringing this to our attention. After reviewing #511, I think it solves the JSON-encoding problem by passing context as a structured array, which is better, but it doesn't solve the faceted search use case in systems like SigNoz. SigNoz (and similar backends) require individual top-level OTLP attributes to support filtering/faceting. A single context attribute containing a nested array/kvlist is not decomposed into searchable facets. The value proposition of #491 is the opt-in The issue with the current merged state is that the non-flatten path in #491 regressed #511's improvement by reverting to json_encode() instead of keeping the structured array. A fix could be:
If you'd like, @arthurmartelli or I can open a follow-up PR that corrects the non-flatten regression while preserving the (optional) flatten feature. |
|
@ChrisLightfootWild please have a look at #460 - it keeps the desired context-flattening while preserving the #511 structured array behavior as default. |
4 participants
Description
Resolves open-telemetry/opentelemetry-php#1837
This PR adds an optional configuration to flatten log context attributes into individual OTLP attributes, improving searchability in observability backends.
Problem
The Laravel LogWatcher currently JSON-encodes the entire log context into a single context attribute:
context: {"http":{"method":"GET","path":"/users","status":200},"user_id":"123"}
This makes it impossible to search/filter by specific values (e.g., http.method = "GET") in backends like SigNoz, Jaeger, or Grafana.
Solution
A new configuration option OTEL_PHP_LARAVEL_LOG_ATTRIBUTES_FLATTEN that, when enabled, spreads context as individual attributes with dot-notation for nested arrays:
http.method: GET
http.path: /users
http.status: 200
user_id: 123
Changes
Backward Compatibility
Testing
./vendor/bin/phpunit --testsuite=unit # 18 tests, 42 assertions
./vendor/bin/phpunit --testsuite=integration # 21 tests, 120 assertions
Screenshot