Skip to content

Commit 09a87bb

Browse files
belisarhbelisarh
authored
fix: double context on custom taps (#4)
1 parent 0bf0da7 commit 09a87bb

File tree

3 files changed

+12
-305
lines changed

3 files changed

+12
-305
lines changed

‎config/logging-monitor.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,18 @@
11
<?php
22

3+
// Monitor uses Laravel's default logging channel (Log::info(), Log::error(), etc.)
4+
// so it works with whatever you have configured as your default channel.
5+
//
6+
// If you want Monitor logs to go to a separate channel, you can:
7+
// 1. Set a different default channel in your logging.php config, OR
8+
// 2. Create a dedicated channel and manually use it like Log::channel('monitor')->info()
9+
//
10+
// The example below shows how you could configure a dedicated channel with Monitor's
11+
// structured logging tap, but this is completely optional.
12+
313
return [
414
'channels' => [
5-
'monitor' => [
15+
'monitor_example' => [
616
'driver' => 'daily',
717
'path' => storage_path('logs/monitor.log'),
818
'level' => 'debug',

‎src/MonitorServiceProvider.php‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public function register(): void
1919
$this->app->singleton(CircuitBreaker::class, fn () => new CircuitBreaker);
2020
$this->app->singleton(ControlledContext::class, fn () => new ControlledContext);
2121

22-
$this->mergeLoggingChannelsFrom(__DIR__.'/../config/logging-monitor.php', 'logging');
22+
// Remove automatic logging channel merging - users should configure their own channels
2323
}
2424

2525
public function boot(): void
@@ -45,16 +45,4 @@ public function boot(): void
4545
app(Trace::class)->start();
4646
}
4747
}
48-
49-
protected function mergeLoggingChannelsFrom(string $path, string $key): void
50-
{
51-
$existingChannels = (array) config("{$key}.channels", []);
52-
53-
/** @var array{channels?: array<string, mixed>} $packageChannels */
54-
$packageChannels = require $path;
55-
56-
config([
57-
"{$key}.channels" => array_merge($packageChannels['channels'] ?? [], $existingChannels),
58-
]);
59-
}
6048
}

‎tests/Feature/MonitorServiceProviderTest.php‎

Lines changed: 0 additions & 291 deletions
Original file line numberDiff line numberDiff line change
@@ -40,89 +40,6 @@ public function test_services_return_correct_types()
4040
->and($this->app->make(LogTimer::class))->toBeInstanceOf(LogTimer::class);
4141
}
4242

43-
public function test_merges_logging_channels_from_package_config()
44-
{
45-
// Set up existing logging config
46-
config(['logging.channels.existing' => ['driver' => 'single']]);
47-
48-
// Re-register the service provider to trigger config merging
49-
$provider = new MonitorServiceProvider($this->app);
50-
$provider->register();
51-
52-
$channels = config('logging.channels');
53-
54-
// Should contain existing channels
55-
expect($channels['existing'])->toBe(['driver' => 'single']);
56-
57-
// Should contain package channels (from logging-monitor.php)
58-
expect($channels)->toHaveKey('monitor');
59-
}
60-
61-
public function test_preserves_existing_logging_channels()
62-
{
63-
// Set up multiple existing channels
64-
config([
65-
'logging.channels.single' => ['driver' => 'single'],
66-
'logging.channels.daily' => ['driver' => 'daily'],
67-
'logging.channels.slack' => ['driver' => 'slack'],
68-
]);
69-
70-
// Re-register to trigger merging
71-
$provider = new MonitorServiceProvider($this->app);
72-
$provider->register();
73-
74-
$channels = config('logging.channels');
75-
76-
// All existing channels should be preserved
77-
expect($channels['single'])->toBe(['driver' => 'single'])
78-
->and($channels['daily'])->toBe(['driver' => 'daily'])
79-
->and($channels['slack'])->toBe(['driver' => 'slack']);
80-
}
81-
82-
public function test_handles_empty_package_channels_config()
83-
{
84-
// Mock the config file to return empty channels
85-
config(['logging.channels.existing' => ['driver' => 'single']]);
86-
87-
// Create a temporary file with empty channels
88-
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
89-
file_put_contents($tempPath, '<?php return ["channels" => []];');
90-
91-
// Use reflection to test the protected method
92-
$provider = new MonitorServiceProvider($this->app);
93-
$reflection = new \ReflectionClass($provider);
94-
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
95-
$method->setAccessible(true);
96-
97-
$method->invoke($provider, $tempPath, 'logging');
98-
99-
// Should preserve existing channels
100-
expect(config('logging.channels.existing'))->toBe(['driver' => 'single']);
101-
102-
unlink($tempPath);
103-
}
104-
105-
public function test_handles_missing_channels_key_in_package_config()
106-
{
107-
config(['logging.channels.existing' => ['driver' => 'single']]);
108-
109-
// Create config file without 'channels' key
110-
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
111-
file_put_contents($tempPath, '<?php return ["other_config" => "value"];');
112-
113-
$provider = new MonitorServiceProvider($this->app);
114-
$reflection = new \ReflectionClass($provider);
115-
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
116-
$method->setAccessible(true);
117-
118-
$method->invoke($provider, $tempPath, 'logging');
119-
120-
// Should preserve existing channels
121-
expect(config('logging.channels.existing'))->toBe(['driver' => 'single']);
122-
123-
unlink($tempPath);
124-
}
125-
12643
public function test_publishes_config_files()
12744
{
12845
// Test that publishing is registered
@@ -247,22 +164,6 @@ public function test_boot_console_logic(): void
247164
expect($trace->hasNotStarted())->toBeTrue();
248165
}
249166

250-
public function test_merge_logging_channels_handles_no_existing_config()
251-
{
252-
// Clear any existing logging config
253-
config(['logging.channels' => null]);
254-
255-
// Re-register to trigger merging
256-
$provider = new MonitorServiceProvider($this->app);
257-
$provider->register();
258-
259-
$channels = config('logging.channels');
260-
261-
// Should have package channels even with no existing config
262-
expect($channels)->toBeArray()
263-
->and($channels)->toHaveKey('monitor');
264-
}
265-
266167
public function test_app_binding_works_through_helper()
267168
{
268169
// Test that app() helper works with our registered services
@@ -292,28 +193,6 @@ public function test_boot_method_integration()
292193
expect($exception)->toBeNull();
293194
}
294195

295-
public function test_config_merging_edge_cases()
296-
{
297-
// Test merging with existing null config
298-
config(['logging.channels' => null]);
299-
300-
$provider = new MonitorServiceProvider($this->app);
301-
$reflection = new \ReflectionClass($provider);
302-
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
303-
$method->setAccessible(true);
304-
305-
// Create a temp config file
306-
$tempPath = tempnam(sys_get_temp_dir(), 'test_logging_monitor');
307-
file_put_contents($tempPath, '<?php return ["channels" => ["test" => ["driver" => "single"]]];');
308-
309-
$method->invoke($provider, $tempPath, 'logging');
310-
311-
// Should set channels even with null existing config
312-
expect(config('logging.channels'))->toBe(['test' => ['driver' => 'single']]);
313-
314-
unlink($tempPath);
315-
}
316-
317196
public function test_service_provider_properties()
318197
{
319198
$provider = new MonitorServiceProvider($this->app);
@@ -323,25 +202,6 @@ public function test_service_provider_properties()
323202
->and(get_class($provider))->toBe(MonitorServiceProvider::class);
324203
}
325204

326-
public function test_merge_logging_channels_with_invalid_file_path()
327-
{
328-
$provider = new MonitorServiceProvider($this->app);
329-
$reflection = new \ReflectionClass($provider);
330-
$method = $reflection->getMethod('mergeLoggingChannelsFrom');
331-
$method->setAccessible(true);
332-
333-
// This should test the error handling for non-existent files
334-
// The method might fail gracefully or throw an exception
335-
try {
336-
$method->invoke($provider, '/non/existent/path.php', 'logging');
337-
// If it doesn't throw, that's also a valid test result
338-
expect(true)->toBeTrue();
339-
} catch (\Exception $e) {
340-
// If it throws, that's expected behavior for invalid paths
341-
expect($e)->toBeInstanceOf(\Exception::class);
342-
}
343-
}
344-
345205
public function test_register_is_complete()
346206
{
347207
// Test the registration process itself by checking that register() method works
@@ -359,156 +219,5 @@ public function test_register_is_complete()
359219
expect($monitor1)->toBe($monitor2)
360220
->and($trace1)->toBe($trace2)
361221
->and($timer1)->toBe($timer2);
362-
363-
// Verify config merging happened during registration
364-
expect(config('logging.channels'))->toHaveKey('monitor');
365-
}
366-
367-
public function test_merge_preserves_complex_channel_structures()
368-
{
369-
// Set up complex existing config with nested arrays
370-
config([
371-
'logging.channels.complex' => [
372-
'driver' => 'stack',
373-
'channels' => ['daily', 'slack'],
374-
'processors' => ['web' => 'App\\Processors\\WebProcessor'],
375-
'tap' => ['App\\Logging\\CustomizeFormatter'],
376-
],
377-
]);
378-
379-
$provider = new MonitorServiceProvider($this->app);
380-
$provider->register();
381-
382-
$channels = config('logging.channels');
383-
384-
// Complex structure should be preserved exactly
385-
expect($channels['complex'])->toBe([
386-
'driver' => 'stack',
387-
'channels' => ['daily', 'slack'],
388-
'processors' => ['web' => 'App\\Processors\\WebProcessor'],
389-
'tap' => ['App\\Logging\\CustomizeFormatter'],
390-
]);
391-
}
392-
393-
public function test_console_boot_conditions_comprehensive()
394-
{
395-
// Test all combinations of console auto-trace conditions
396-
$testCases = [
397-
['console' => true, 'testing' => false, 'started' => false, 'should_start' => true],
398-
['console' => true, 'testing' => true, 'started' => false, 'should_start' => false],
399-
['console' => false, 'testing' => false, 'started' => false, 'should_start' => false],
400-
['console' => true, 'testing' => false, 'started' => true, 'should_start' => false],
401-
];
402-
403-
foreach ($testCases as $case) {
404-
$trace = new Trace;
405-
if ($case['started']) {
406-
$trace->start();
407-
}
408-
409-
$this->app->instance(Trace::class, $trace);
410-
411-
// Create custom provider to test the exact boolean logic
412-
$provider = new class($this->app) extends MonitorServiceProvider
413-
{
414-
public function test_console_logic(bool $console, bool $testing, bool $hasStarted): void
415-
{
416-
if ($console && ! $testing && ! $hasStarted) {
417-
app(Trace::class)->start();
418-
}
419-
}
420-
};
421-
422-
$originalStarted = $trace->hasStarted();
423-
$provider->test_console_logic($case['console'], $case['testing'], $case['started']);
424-
425-
if ($case['should_start']) {
426-
expect($trace->hasStarted())->toBeTrue(
427-
'Trace should be started for case: '.json_encode($case)
428-
);
429-
} else {
430-
expect($trace->hasStarted())->toBe($originalStarted,
431-
'Trace state should be unchanged for case: '.json_encode($case)
432-
);
433-
}
434-
}
435-
}
436-
437-
public function test_console_auto_trace_executes_with_testing_enabled()
438-
{
439-
// Enable console auto-trace in testing environment
440-
config(['monitor.console_auto_trace.enable_in_testing' => true]);
441-
442-
// Create a fresh trace that hasn't started
443-
$trace = new Trace;
444-
expect($trace->hasNotStarted())->toBeTrue();
445-
446-
// Bind to container
447-
$this->app->instance(Trace::class, $trace);
448-
449-
// Create the actual service provider and boot it
450-
$provider = new MonitorServiceProvider($this->app);
451-
$provider->boot();
452-
453-
// The trace should now be started because we enabled it in testing
454-
expect($trace->hasStarted())->toBeTrue();
455-
}
456-
457-
public function test_console_auto_trace_respects_disabled_config()
458-
{
459-
// Disable console auto-trace entirely
460-
config(['monitor.console_auto_trace.enabled' => false]);
461-
462-
// Create a fresh trace
463-
$trace = new Trace;
464-
expect($trace->hasNotStarted())->toBeTrue();
465-
466-
$this->app->instance(Trace::class, $trace);
467-
468-
// Boot the provider
469-
$provider = new MonitorServiceProvider($this->app);
470-
$provider->boot();
471-
472-
// Trace should NOT be started because auto-trace is disabled
473-
expect($trace->hasNotStarted())->toBeTrue();
474-
}
475-
476-
public function test_console_auto_trace_respects_already_started_trace()
477-
{
478-
// Enable auto-trace in testing
479-
config(['monitor.console_auto_trace.enable_in_testing' => true]);
480-
481-
// Create and start a trace
482-
$trace = new Trace;
483-
$trace->start();
484-
$originalId = $trace->id();
485-
486-
$this->app->instance(Trace::class, $trace);
487-
488-
// Boot the provider
489-
$provider = new MonitorServiceProvider($this->app);
490-
$provider->boot();
491-
492-
// Trace should remain the same (not restarted)
493-
expect($trace->hasStarted())->toBeTrue()
494-
->and($trace->id())->toBe($originalId);
495-
}
496-
497-
public function test_console_auto_trace_config_defaults()
498-
{
499-
// Test that default config values work as expected
500-
$trace = new Trace;
501-
expect($trace->hasNotStarted())->toBeTrue();
502-
503-
$this->app->instance(Trace::class, $trace);
504-
505-
// Clear any existing config to test defaults
506-
config(['monitor.console_auto_trace' => []]);
507-
508-
$provider = new MonitorServiceProvider($this->app);
509-
$provider->boot();
510-
511-
// With defaults, auto-trace is enabled but not in testing
512-
expect($trace->hasNotStarted())->toBeTrue();
513222
}
514223
}

0 commit comments

Comments
 (0)