open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.23k stars 765 forks source link

[api] Optimise `TraceContextPropagator.Extract` #5749

Closed stevejgordon closed 3 months ago

stevejgordon commented 4 months ago

Contributes to #728

Changes

This PR removes almost all overhead of extracting the tracestate inside the TraceContextPropagator by switching to a stack-allocated or rented buffer when building the final string. Specifically, this avoids using StringBuilder and HashSet to prevent heap allocations. The execution time is also slightly improved with this approach. I've preserved the existing behaviour based on the tests.

This also adds a benchmark to measure the performance of the extract method.

| Method     | LongListMember | MembersCount | Mean        | Error     | StdDev    | Ratio | RatioSD | Gen0    | Gen1   | Allocated | Alloc Ratio |
|----------- |--------------- |------------- |------------:|----------:|----------:|------:|--------:|--------:|-------:|----------:|------------:|
| Extract    | False          | 0            |    128.2 ns |   2.02 ns |   1.79 ns |  1.00 |    0.00 |  0.0312 |      - |     392 B |        1.00 |
| ExtractNew | False          | 0            |    122.5 ns |   2.20 ns |   2.62 ns |  0.96 |    0.02 |  0.0176 |      - |     224 B |        0.57 |
|            |                |              |             |           |           |       |         |         |        |           |             |
| Extract    | False          | 4            |    551.5 ns |  10.68 ns |  14.61 ns |  1.00 |    0.00 |  0.2108 | 0.0010 |    2656 B |        1.00 |
| ExtractNew | False          | 4            |    309.9 ns |   3.43 ns |   2.86 ns |  0.56 |    0.02 |  0.0463 |      - |     584 B |        0.22 |
|            |                |              |             |           |           |       |         |         |        |           |             |
| Extract    | False          | 32           |  3,419.8 ns | 145.07 ns | 420.86 ns |  1.00 |    0.00 |  1.2512 | 0.0381 |   15704 B |        1.00 |
| ExtractNew | False          | 32           |  2,841.3 ns |  54.61 ns |  76.56 ns |  0.83 |    0.05 |  0.2327 |      - |    2936 B |        0.19 |
|            |                |              |             |           |           |       |         |         |        |           |             |
| Extract    | True           | 0            |    129.5 ns |   2.47 ns |   3.29 ns |  1.00 |    0.00 |  0.0312 |      - |     392 B |        1.00 |
| ExtractNew | True           | 0            |    131.5 ns |   2.13 ns |   1.99 ns |  1.02 |    0.03 |  0.0176 |      - |     224 B |        0.57 |
|            |                |              |             |           |           |       |         |         |        |           |             |
| Extract    | True           | 4            |  3,052.5 ns |  60.07 ns |  59.00 ns |  1.00 |    0.00 |  1.5640 | 0.0648 |   19648 B |        1.00 |
| ExtractNew | True           | 4            |  1,972.3 ns |  38.22 ns |  35.75 ns |  0.65 |    0.03 |  0.3471 | 0.0038 |    4360 B |        0.22 |
|            |                |              |             |           |           |       |         |         |        |           |             |
| Extract    | True           | 32           | 20,789.8 ns | 409.46 ns | 438.11 ns |  1.00 |    0.00 | 10.6812 | 2.1057 |  134392 B |        1.00 |
| ExtractNew | True           | 32           | 15,939.3 ns | 313.31 ns | 505.94 ns |  0.76 |    0.03 |  2.6245 | 0.3204 |   33147 B |        0.25 |

Merge requirement checklist

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 94.73684% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.22%. Comparing base (6250307) to head (d43e26f). Report is 288 commits behind head on main.

Files Patch % Lines
....Api/Context/Propagation/TraceContextPropagator.cs 94.73% 3 Missing :warning:
Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/graphs/tree.svg?width=650&height=150&src=pr&token=vscyfvPfy5&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #5749 +/- ## ========================================== + Coverage 83.38% 86.22% +2.84% ========================================== Files 297 256 -41 Lines 12531 11140 -1391 ========================================== - Hits 10449 9606 -843 + Misses 2082 1534 -548 ``` | [Flag](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `?` | | | [unittests-Project-Experimental](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `86.17% <94.73%> (?)` | | | [unittests-Project-Stable](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | `86.25% <94.73%> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [....Api/Context/Propagation/TraceContextPropagator.cs](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749?src=pr&el=tree&filepath=src%2FOpenTelemetry.Api%2FContext%2FPropagation%2FTraceContextPropagator.cs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c3JjL09wZW5UZWxlbWV0cnkuQXBpL0NvbnRleHQvUHJvcGFnYXRpb24vVHJhY2VDb250ZXh0UHJvcGFnYXRvci5jcw==) | `90.82% <94.73%> (+1.35%)` | :arrow_up: | ... and [201 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-dotnet/pull/5749/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
github-actions[bot] commented 3 months ago

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

stevejgordon commented 3 months ago

@CodeBlanch, I am just bumping here to keep this open and see if we can solve the discussion on using stackalloc offline to move this along? I had hoped to attend the SIG to raise it, but the timing didn't work out again last night.

CodeBlanch commented 3 months ago

Sorry for the delay on this @stevejgordon! We did look at this on the SIG yesterday. @vishweshbankwar will do a review pass when he has a moment and then we'll go from there.