-
Notifications
You must be signed in to change notification settings - Fork 802
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
[Exporter.OTLP] Optimize GetHeaders by Spans #6179
base: main
Are you sure you want to change the base?
[Exporter.OTLP] Optimize GetHeaders by Spans #6179
Conversation
…thod and reducing allocations. OtlpExporterOptionsExtensions.cs
@nimanikoo Could you please add a benchmark to measure performance improvement? |
Sure! I’ll add a benchmark to measure the performance improvement. I’ll use BenchmarkDotNet to compare the old and new implementations of GetHeaders. The benchmark will measure execution time and memory allocations to demonstrate the benefits of using Span. I’ll update the PR soon with the benchmark results. Thanks for the suggestion! |
@rajkumar-rangaraj I benchmarked two implementations of the System Specifications:
Benchmark Results:
These results indicate that |
I modified only GetHeaders method, making changes inside the method without altering its input parameters or return result. However, I'm unable to identify the issue causing the tests and build to fail. Could you please help me in resolving this? |
In your local development environment, performing a release build should reveal these errors. Here are the errors from CI:
|
Ah, I figured out what the problem was. |
- Ensure consistent indentation to comply with StyleCop rule SA1137. - Remove multiple consecutive blank lines to comply with StyleCop rule SA1507.
I've tried to fix all the indentation issues mentioned by the code analyzer. Is there anything else I should fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor improvement and one direct build fix.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptionsExtensions.cs
Outdated
Show resolved
Hide resolved
…ptionsExtensions.cs Co-authored-by: Piotr Kiełkowicz <[email protected]>
throw new ArgumentException("Headers provided in an invalid format."); | ||
} | ||
pair = headersSpan; | ||
headersSpan = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
headersSpan = []; | |
headersSpan = ReadOnlySpan<char>.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? The []
is the recommended way by the analyzers. IDE0300/IDE0301 as I remember?
Co-authored-by: xiang17 <[email protected]>
The benchmarks results look good. However, it doesn't have any memory related stats. Can you run with the |
How much code coverage do the existing unit test cases have for this method? Since the sole purpose of this PR is to update the parse logic, it might be good to add more test cases. Can you also include the benchmark code in the PR description or somewhere in the comment? No need to include it in the code change. |
Based on the references used by this method, I’ve run benchmarks in two different scenarios with the same conditions and system as mentioned in the previous comments, and this is the result. Do you think any further changes are needed or is there anything else I should do? old code new code |
Title: Optimize header parsing in
GetHeaders
method by replacingSplit
withSpan
to enhance performance and reduce allocations. OtlpExporterOptionsExtensions.csDesign discussion issue: 6180
#6180
Changes
Refactored the
GetHeaders
method to utilizeSpan<T>
for parsing header strings, aiming to improve performance by minimizing memory allocations. This change is inspired by techniques discussed in the following articles:By implementing these optimizations, I hope to contribute to the project's performance enhancements and am eager to assist further in its development.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes