Skip to content

Use array lookup instead of recursion in Context#821

Merged
bobstrecansky merged 3 commits into
open-telemetry:mainfrom
Nevay:feature/context-non-recursive
Sep 14, 2022
Merged

Use array lookup instead of recursion in Context#821
bobstrecansky merged 3 commits into
open-telemetry:mainfrom
Nevay:feature/context-non-recursive

Conversation

@Nevay

@Nevay Nevay commented Sep 10, 2022

Copy link
Copy Markdown
Contributor

Prevents leaking memory if Context::with() is called iteratively; following example throws an out of memory error with the recursive implementation:

$contextKey = Context::createKey('-');
$context = Context::getCurrent();
for ($i = 0;; $i++) {
    $context = $context->with($contextKey, $i);
}

@Nevay
Nevay requested a review from a team September 10, 2022 14:10
@codecov

codecov Bot commented Sep 10, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #821 (3fc150f) into main (ad3ccd8) will increase coverage by 0.21%.
The diff coverage is 85.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #821      +/-   ##
============================================
+ Coverage     82.82%   83.04%   +0.21%     
+ Complexity     1829     1828       -1     
============================================
  Files           223      222       -1     
  Lines          4723     4725       +2     
============================================
+ Hits           3912     3924      +12     
+ Misses          811      801      -10
Flag Coverage Δ
7.4 83.06% <85.00%> (+0.22%) ⬆️
8.0 83.08% <85.00%> (+0.21%) ⬆️
8.1 83.08% <85.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/API/Trace/AbstractSpan.php 0.00% <0.00%> (ø)
src/Context/Context.php 88.88% <87.50%> (+8.88%) ⬆️
src/API/Baggage/Baggage.php 83.87% <100.00%> (ø)
src/Context/ContextKeys.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad3ccd8...3fc150f. Read the comment docs.

Nevay
Nevay commented Sep 10, 2022
View reviewed changes
Comment thread src/Context/Context.php Outdated
brettmc
brettmc reviewed Sep 11, 2022
View reviewed changes
Comment thread src/Context/Context.php
@Nevay
Nevay force-pushed the feature/context-non-recursive branch from 489d44d to f319f52 Compare September 13, 2022 18:01
@Nevay
Nevay force-pushed the feature/context-non-recursive branch from acabc72 to 3fc150f Compare September 13, 2022 18:38
@bobstrecansky
bobstrecansky merged commit 6196d7a into open-telemetry:main Sep 14, 2022
@Nevay
Nevay deleted the feature/context-non-recursive branch April 19, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bobstrecansky bobstrecansky bobstrecansky approved these changes
@brettmc brettmc brettmc approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Nevay @bobstrecansky @brettmc