Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(otel): Missing addLink method and Fiber handling #2849

Merged
merged 10 commits into from
Sep 17, 2024

Conversation

PROFeNoM
Copy link
Contributor

@PROFeNoM PROFeNoM commented Sep 13, 2024

Description

  • SpanInterface::addLink was added with Add Span::addLink() and Meter::createGauge() open-telemetry/opentelemetry-php#1289 and is included in 1.1+. It is currently missing from our shim. For backward compatibility reasons, ImmutableSpan will take a new argument, totalRecordedLinks if the latter method exists.
  • Improve fiber bound context storage open-telemetry/opentelemetry-php#1270 modified Fiber bound context storage, and hence required changes to Context::storage. For backward compatibility reasons, we have to keep both initializations.
  • To proactively spot such changes and ensure backward compatibility, the test_opentelemetry_1 suite (currently: @stable) will be split into two: @beta and 1.0.*.
  • There are some white space changes in the PR 😬 Sorry, I didn't manage to remove them :-(
  • I just changed the Fiber test back to .phpt; it was driving me crazy and I couldn't otherwise properly reset the context between tests (...I didn't have time to look further into it). This way, it's appropriately reset :-)

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@PROFeNoM PROFeNoM added 🐛 bug Something isn't working cat:opentelemetry labels Sep 13, 2024
@PROFeNoM PROFeNoM self-assigned this Sep 13, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 21 lines in your changes missing coverage. Please review.

Project coverage is 80.94%. Comparing base (31382ec) to head (e44f4e4).

Files with missing lines Patch % Lines
src/DDTrace/OpenTelemetry/Span.php 59.09% 18 Missing ⚠️
src/DDTrace/OpenTelemetry/Context.php 40.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2849      +/-   ##
============================================
- Coverage     81.05%   80.94%   -0.11%     
- Complexity     2517     2526       +9     
============================================
  Files           146      146              
  Lines         14654    14680      +26     
============================================
+ Hits          11878    11883       +5     
- Misses         2776     2797      +21     
Flag Coverage Δ
tracer-extension 78.18% <ø> (ø)
tracer-php 82.10% <57.14%> (-0.16%) ⬇️

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

Files with missing lines Coverage Δ
src/DDTrace/OpenTelemetry/Context.php 82.81% <40.00%> (-3.48%) ⬇️
src/DDTrace/OpenTelemetry/Span.php 85.42% <59.09%> (-6.58%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@PROFeNoM PROFeNoM changed the title fix(otel): Missing method and Fiber handling fix(otel): Missing addLink method and Fiber handling Sep 13, 2024
@pr-commenter
Copy link

pr-commenter bot commented Sep 13, 2024

Benchmarks

Benchmark execution time: 2024-09-16 08:08:18

Comparing candidate commit e4b186a in PR branch alex/fix/otel-addlink-fiber with baseline commit 1a7182c in branch master.

Found 3 performance improvements and 5 performance regressions! Performance is the same for 170 metrics, 0 unstable metrics.

scenario:LaravelBench/benchLaravelBaseline-opcache

  • 🟥 execution_time [+86.391µs; +206.589µs] or [+2.856%; +6.830%]

scenario:PDOBench/benchPDOBaseline

  • 🟩 execution_time [-13.842µs; -11.256µs] or [-7.187%; -5.844%]

scenario:PDOBench/benchPDOOverheadWithDBM

  • 🟩 execution_time [-14.699µs; -12.174µs] or [-4.740%; -3.926%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2

  • 🟥 execution_time [+333.793ns; +713.207ns] or [+4.872%; +10.410%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

  • 🟥 execution_time [+149.073ns; +441.727ns] or [+2.140%; +6.340%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4-opcache

  • 🟥 execution_time [+142.816ns; +503.984ns] or [+2.102%; +7.419%]

scenario:TraceSerializationBench/benchSerializeTrace

  • 🟥 execution_time [+3.996µs; +7.604µs] or [+2.374%; +4.518%]

scenario:WordPressBench/benchWordPressOverhead

  • 🟩 execution_time [-21.847ms; -21.541ms] or [-83.051%; -81.888%]

@PROFeNoM PROFeNoM marked this pull request as ready for review September 13, 2024 13:43
@PROFeNoM PROFeNoM requested review from a team as code owners September 13, 2024 13:43
@PROFeNoM PROFeNoM marked this pull request as draft September 16, 2024 07:13
@PROFeNoM
Copy link
Contributor Author

I'm putting it back to draft. +7-9% performance regression for a simple check is simply too much. I'll try and optimize that a little bit.

@bwoebi
Copy link
Collaborator

bwoebi commented Sep 16, 2024

@PROFeNoM Feels like the benchmarks are simply flaky. ... But I suppose for that one the micro-optimization is worth it.

@pr-commenter
Copy link

pr-commenter bot commented Sep 16, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2024-09-16 13:24:15

Comparing candidate commit e44f4e4 in PR branch alex/fix/otel-addlink-fiber with baseline commit 31382ec in branch master.

Found 2 performance improvements and 4 performance regressions! Performance is the same for 172 metrics, 0 unstable metrics.

scenario:ContextPropagationBench/benchInject64Bit

  • 🟥 execution_time [+188.236ns; +617.764ns] or [+2.531%; +8.307%]

scenario:ContextPropagationBench/benchInject64Bit-opcache

  • 🟥 execution_time [+183.937ns; +568.063ns] or [+2.446%; +7.554%]

scenario:LogsInjectionBench/benchLogsInfoInjection-opcache

  • 🟩 execution_time [-439.363ns; -243.637ns] or [-5.657%; -3.137%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟥 execution_time [+9.677µs; +14.422µs] or [+5.432%; +8.096%]

scenario:PDOBench/benchPDOOverhead-opcache

  • 🟥 execution_time [+14.523µs; +17.533µs] or [+5.064%; +6.114%]

scenario:WordPressBench/benchWordPressOverhead

  • 🟩 execution_time [-21.839ms; -21.599ms] or [-82.871%; -81.961%]

@PROFeNoM PROFeNoM marked this pull request as ready for review September 16, 2024 14:18
Copy link
Collaborator

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good :-)

@PROFeNoM PROFeNoM merged commit 17b3c7a into master Sep 17, 2024
644 of 646 checks passed
@PROFeNoM PROFeNoM deleted the alex/fix/otel-addlink-fiber branch September 17, 2024 06:20
@github-actions github-actions bot added this to the 1.4.0 milestone Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working cat:opentelemetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants