Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions src/State/Scope.php
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,69 @@
$this->propagationContext = $propagationContext ?? PropagationContext::fromDefaults();
}

/**
* Merges the process-global scope underneath the current isolation scope.
*
* The returned scope is transient and should be used for one event capture.
*
* @internal
*/
public static function mergeScopes(self $globalScope, self $isolationScope): self
{
$mergedScope = clone $isolationScope;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The mergeScopes function performs a shallow clone of the scope, causing the span object to be shared between the original and merged scopes, which can lead to side effects.
Severity: LOW

Suggested Fix

Update the Scope::__clone() method to perform a deep clone of the $span property. This will ensure that merged scopes have their own independent span instance, preventing unintended side effects if the span is mutated after the merge operation. Additionally, add a test case to verify the behavior when the isolation scope has a span.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/State/Scope.php#L124

Potential issue: The `mergeScopes()` function creates a new scope by cloning an existing
`isolationScope`. However, the `Scope::__clone()` method performs a shallow copy and
does not clone the `span` object. As a result, the new `mergedScope` and the original
`isolationScope` share the same `span` instance. If the span is mutated in one scope,
the change will unexpectedly affect the other. While the current `applyToEvent()` usage
is read-only, any future code that mutates the span after merging could lead to
incorrect trace data. This potential issue is not covered by tests, as the relevant test
case only handles scenarios where the isolation scope has no span.

Did we get this right? 👍 / 👎 to inform future reviews.


$mergedScope->tags = array_merge($globalScope->tags, $isolationScope->tags);
$mergedScope->extra = array_merge($globalScope->extra, $isolationScope->extra);
$mergedScope->contexts = array_merge($globalScope->contexts, $isolationScope->contexts);

if ($globalScope->user !== null && $isolationScope->user !== null) {
$mergedScope->user = (clone $globalScope->user)->merge($isolationScope->user);
} elseif ($globalScope->user !== null) {
$mergedScope->user = clone $globalScope->user;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global user revives cleared isolation

Medium Severity

When the isolation scope has no user (including after removeUser()), mergeScopes still copies the global scope user onto the merged scope. Applying that scope can attach global user data to events that the isolation scope intentionally left anonymous.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 15faea1. Configure here.


$mergedScope->level = $isolationScope->level ?? $globalScope->level;
$mergedScope->fingerprint = array_merge($globalScope->fingerprint, $isolationScope->fingerprint);
$mergedScope->breadcrumbs = \array_slice(array_merge($globalScope->breadcrumbs, $isolationScope->breadcrumbs), -100);
$mergedScope->flags = self::mergeFlags($globalScope->flags, $isolationScope->flags);
$mergedScope->attachments = array_merge($globalScope->attachments, $isolationScope->attachments);
$mergedScope->eventProcessors = array_merge($globalScope->eventProcessors, $isolationScope->eventProcessors);

return $mergedScope;
}

/**
* @param array<int, array<string, bool>> $globalFlags
* @param array<int, array<string, bool>> $isolationFlags
*
* @return array<int, array<string, bool>>
*/
private static function mergeFlags(array $globalFlags, array $isolationFlags): array
{
$flagsByKey = [];

foreach (array_merge($globalFlags, $isolationFlags) as $flag) {
$flagKey = key($flag);

if ($flagKey === null) {
continue;
}

unset($flagsByKey[$flagKey]);
$flagsByKey[$flagKey] = (bool) current($flag);

Check notice on line 164 in src/State/Scope.php

View workflow job for this annotation

GitHub Actions / Mago

redundant-cast

Redundant cast to `(bool)`: the expression already has this type. >This expression already has type `bool`. Casting a value to a type it already possesses has no effect. Help: Remove the redundant `(bool)` cast.
}

$flagsByKey = \array_slice($flagsByKey, -self::MAX_FLAGS, self::MAX_FLAGS, true);

$flags = [];

foreach ($flagsByKey as $flagKey => $flagResult) {
$flags[] = [$flagKey => $flagResult];
}

return $flags;
}

/**
* Sets a new tag in the tags context.
*
Expand Down
227 changes: 227 additions & 0 deletions tests/State/ScopeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,233 @@ public function testApplyToEvent(): void
$this->assertSame('566e3688a61d4bc888951642d6f14a19', $dynamicSamplingContext->get('trace_id'));
}

public function testMergeScopesAppliesGlobalScopeUnderIsolationScope(): void
{
$globalBreadcrumb = new Breadcrumb(Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'global');
$isolationBreadcrumb = new Breadcrumb(Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'isolation');
$globalAttachment = Attachment::fromBytes('global.txt', 'global');
$isolationAttachment = Attachment::fromBytes('isolation.txt', 'isolation');

$globalUser = UserDataBag::createFromUserIdentifier('global-user');
$globalUser->setMetadata('shared', 'global');
$globalUser->setMetadata('global', true);

$globalScope = new Scope();
$globalScope->setTag('shared', 'global');
$globalScope->setTag('global', 'tag');
$globalScope->setExtra('shared', 'global');
$globalScope->setExtra('global', true);
$globalScope->setContext('shared_context', ['value' => 'global']);
$globalScope->setContext('global_context', ['value' => 'global']);
$globalScope->setUser($globalUser);
$globalScope->setLevel(Severity::error());
$globalScope->setFingerprint(['global-fingerprint']);
$globalScope->addBreadcrumb($globalBreadcrumb);
$globalScope->addFeatureFlag('shared-flag', false);
$globalScope->addFeatureFlag('global-flag', true);
$globalScope->addAttachment($globalAttachment);

$isolationUser = UserDataBag::createFromUserIdentifier('isolation-user');
$isolationUser->setMetadata('shared', 'isolation');
$isolationUser->setMetadata('isolation', true);

$isolationScope = new Scope();
$isolationScope->setTag('shared', 'isolation');
$isolationScope->setTag('isolation', 'tag');
$isolationScope->setExtra('shared', 'isolation');
$isolationScope->setExtra('isolation', true);
$isolationScope->setContext('shared_context', ['value' => 'isolation']);
$isolationScope->setContext('isolation_context', ['value' => 'isolation']);
$isolationScope->setUser($isolationUser);
$isolationScope->setLevel(Severity::warning());
$isolationScope->setFingerprint(['isolation-fingerprint']);
$isolationScope->addBreadcrumb($isolationBreadcrumb);
$isolationScope->addFeatureFlag('shared-flag', true);
$isolationScope->addFeatureFlag('isolation-flag', false);
$isolationScope->addAttachment($isolationAttachment);

$eventUser = UserDataBag::createFromUserIdentifier('event-user');
$eventUser->setMetadata('shared', 'event');
$eventUser->setMetadata('event', true);

$event = Event::createEvent();
$event->setTag('shared', 'event');
$event->setTag('event', 'tag');
$event->setExtra(['shared' => 'event', 'event' => true]);
$event->setContext('shared_context', ['value' => 'event']);
$event->setUser($eventUser);
$event->setFingerprint(['event-fingerprint']);

$event = Scope::mergeScopes($globalScope, $isolationScope)->applyToEvent($event);

$this->assertNotNull($event);
$this->assertTrue($event->getLevel()->isEqualTo(Severity::warning()));
$this->assertSame(['event-fingerprint', 'global-fingerprint', 'isolation-fingerprint'], $event->getFingerprint());
$this->assertSame([
'shared' => 'event',
'global' => 'tag',
'isolation' => 'tag',
'event' => 'tag',
], $event->getTags());
$this->assertSame([
'shared' => 'event',
'global' => true,
'isolation' => true,
'event' => true,
], $event->getExtra());
$this->assertSame(['value' => 'event'], $event->getContexts()['shared_context']);
$this->assertSame(['value' => 'global'], $event->getContexts()['global_context']);
$this->assertSame(['value' => 'isolation'], $event->getContexts()['isolation_context']);
$this->assertSame([
'values' => [
[
'flag' => 'global-flag',
'result' => true,
],
[
'flag' => 'shared-flag',
'result' => true,
],
[
'flag' => 'isolation-flag',
'result' => false,
],
],
], $event->getContexts()['flags']);
$this->assertSame([$globalBreadcrumb, $isolationBreadcrumb], $event->getBreadcrumbs());
$this->assertSame([$globalAttachment, $isolationAttachment], $event->getAttachments());

$user = $event->getUser();
$this->assertNotNull($user);
$this->assertSame('event-user', $user->getId());
$this->assertSame([
'shared' => 'event',
'global' => true,
'isolation' => true,
'event' => true,
], $user->getMetadata());
}

public function testMergeScopesUsesGlobalLevelWhenIsolationLevelIsUnset(): void
{
$globalScope = new Scope();
$globalScope->setLevel(Severity::error());

$event = Scope::mergeScopes($globalScope, new Scope())->applyToEvent(Event::createEvent());

$this->assertNotNull($event);
$this->assertTrue($event->getLevel()->isEqualTo(Severity::error()));
}

public function testMergeScopesCapsBreadcrumbsAndFlags(): void
{
$globalScope = new Scope();
$globalBreadcrumbs = [];

foreach (range(1, 100) as $i) {
$breadcrumb = new Breadcrumb(Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, "global{$i}");
$globalBreadcrumbs[] = $breadcrumb;
$globalScope->addBreadcrumb($breadcrumb);
$globalScope->addFeatureFlag("feature{$i}", true);
}

$isolationBreadcrumb = new Breadcrumb(Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'isolation');
$isolationScope = new Scope();
$isolationScope->addBreadcrumb($isolationBreadcrumb);
$isolationScope->addFeatureFlag('feature50', false);
$isolationScope->addFeatureFlag('feature101', true);

$event = Scope::mergeScopes($globalScope, $isolationScope)->applyToEvent(Event::createEvent());

$this->assertNotNull($event);
$this->assertCount(100, $event->getBreadcrumbs());
$this->assertSame($globalBreadcrumbs[1], $event->getBreadcrumbs()[0]);
$this->assertSame($isolationBreadcrumb, $event->getBreadcrumbs()[99]);

$flags = $event->getContexts()['flags']['values'];
$this->assertCount(Scope::MAX_FLAGS, $flags);
$this->assertSame([
'flag' => 'feature2',
'result' => true,
], $flags[0]);
$this->assertSame([
'flag' => 'feature50',
'result' => false,
], $flags[98]);
$this->assertSame([
'flag' => 'feature101',
'result' => true,
], $flags[99]);
$this->assertFalse(\in_array('feature1', array_column($flags, 'flag'), true));
}

public function testMergeScopesKeepsTraceStateFromIsolationScope(): void
{
$globalPropagationContext = PropagationContext::fromDefaults();
$globalPropagationContext->setTraceId(new TraceId('11111111111111111111111111111111'));
$globalPropagationContext->setSpanId(new SpanId('1111111111111111'));

$globalSpan = new Span();
$globalSpan->setTraceId(new TraceId('22222222222222222222222222222222'));
$globalSpan->setSpanId(new SpanId('2222222222222222'));

$globalScope = new Scope($globalPropagationContext);
$globalScope->setSpan($globalSpan);

$isolationPropagationContext = PropagationContext::fromDefaults();
$isolationPropagationContext->setTraceId(new TraceId('33333333333333333333333333333333'));
$isolationPropagationContext->setSpanId(new SpanId('3333333333333333'));

$isolationScope = new Scope($isolationPropagationContext);

$mergedScope = Scope::mergeScopes($globalScope, $isolationScope);

$this->assertNull($mergedScope->getSpan());
$this->assertNotSame($isolationScope->getPropagationContext(), $mergedScope->getPropagationContext());
$this->assertSame([
'trace_id' => '33333333333333333333333333333333',
'span_id' => '3333333333333333',
], $mergedScope->getTraceContext());

$event = $mergedScope->applyToEvent(Event::createEvent());

$this->assertNotNull($event);
$this->assertSame([
'trace_id' => '33333333333333333333333333333333',
'span_id' => '3333333333333333',
], $event->getContexts()['trace']);
}

public function testMergeScopesKeepsProcessorOrder(): void
{
$calls = [];

Scope::addGlobalEventProcessor(static function (Event $event) use (&$calls): ?Event {
$calls[] = 'static';

return $event;
});

$globalScope = new Scope();
$globalScope->addEventProcessor(static function (Event $event) use (&$calls): ?Event {
$calls[] = 'global';

return $event;
});

$isolationScope = new Scope();
$isolationScope->addEventProcessor(static function (Event $event) use (&$calls): ?Event {
$calls[] = 'isolation';

return $event;
});

$event = Scope::mergeScopes($globalScope, $isolationScope)->applyToEvent(Event::createEvent());

$this->assertNotNull($event);
$this->assertSame(['static', 'global', 'isolation'], $calls);
}

/**
* @dataProvider eventWithLogCountProvider
*/
Expand Down