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.

Sign up for GitHub

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

Jump to bottom

Change multiBatcher to use sync.Map, avoid global lock on fast path #7714

Merged
merged 1 commit into from
May 23, 2023
Merged

Change multiBatcher to use sync.Map, avoid global lock on fast path #7714

merged 1 commit into from
May 23, 2023

Conversation

bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu bogdandrutu requested a review from a team as a code owner May 23, 2023 01:20
@bogdandrutu
Change multiBatcher to use sync.Map, avoid global lock on fast path
1218853 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Patch coverage: 94.59% and project coverage change: +0.03 🎉

Comparison is base (179d4be) 90.99% compared to head (1218853) 91.02%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7714      +/-   ##
==========================================
+ Coverage   90.99%   91.02%   +0.03%     
==========================================
  Files         295      295              
  Lines       14548    14535      -13     
==========================================
- Hits        13238    13231       -7     
+ Misses       1046     1042       -4     
+ Partials      264      262       -2
Impacted Files Coverage Δ
processor/batchprocessor/batch_processor.go 93.60% <94.59%> (+1.85%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

codeboten
codeboten approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

This looks ok to me, do you have any benchmarks before/after the change?

@codeboten
Copy link
Contributor

codeboten commented May 23, 2023 •
edited

Ran a quick benchmark, the results look good. Before the change:

BenchmarkTraceSizeSpanCount-10                  465039222                2.546 ns/op
BenchmarkBatchMetricProcessor-10                   10000            142466 ns/op
BenchmarkMultiBatchMetricProcessor-10              17671            232864 ns/op

After the change:

BenchmarkTraceSizeSpanCount-10                  457569194                2.526 ns/op
BenchmarkBatchMetricProcessor-10                   10000            107473 ns/op
BenchmarkMultiBatchMetricProcessor-10              17484            200479 ns/op

@codeboten codeboten merged commit e5a1330 into open-telemetry:main May 23, 2023
28 of 29 checks passed
@github-actions github-actions bot added this to the next release milestone May 23, 2023
@bogdandrutu bogdandrutu deleted the sharder branch May 23, 2023 15:50
@bogdandrutu bogdandrutu mentioned this pull request May 23, 2023
Merged
@andrzej-stencel andrzej-stencel mentioned this pull request Mar 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@jmacd jmacd jmacd approved these changes

@dmitryax dmitryax dmitryax approved these changes

@djaglowski djaglowski Awaiting requested review from djaglowski djaglowski is a code owner automatically assigned from open-telemetry/collector-approvers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.78.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bogdandrutu @codeboten @jmacd @dmitryax