Skip to content

Fix shutdown order for collector components#2381

Merged
jpkrohling merged 2 commits intojaegertracing:masterfrom
Sreevani871:master
Aug 11, 2020
Merged

Fix shutdown order for collector components#2381
jpkrohling merged 2 commits intojaegertracing:masterfrom
Sreevani871:master

Conversation

@Sreevani871
Copy link
Copy Markdown
Contributor

@Sreevani871 Sreevani871 commented Aug 10, 2020

bug fix: Sequence of shutdown calls resulting in crash due to panic and leads to span loss to avoid span loss

Which problem is this PR solving?

Short description of the changes

  • Changes include reordering of the shutdown sequence as follows to allow collector to stop accepting new spans and drain the queue to avoid span loss.
    servers -> queue processors (with drain) -> writers

@Sreevani871 Sreevani871 requested a review from a team as a code owner August 10, 2020 17:03
…ose to avoid span loss

Signed-off-by: Sreevani871 <sreevani@freshdesk.com>
@yurishkuro yurishkuro changed the title bug fix: Sequence of shutdown calls resulting in crash due to panic and leads to span loss Fix shutdown order for collector components Aug 11, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 11, 2020

Codecov Report

Merging #2381 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2381   +/-   ##
=======================================
  Coverage   95.60%   95.61%           
=======================================
  Files         206      206           
  Lines       10548    10548           
=======================================
+ Hits        10084    10085    +1     
+ Misses        396      395    -1     
  Partials       68       68           
Impacted Files Coverage Δ
cmd/flags/admin.go 77.77% <0.00%> (-1.59%) ⬇️
...lugin/sampling/strategystore/adaptive/processor.go 100.00% <0.00%> (+0.79%) ⬆️

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 afe491f...587f664. Read the comment docs.

Copy link
Copy Markdown
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM!

@jpkrohling jpkrohling merged commit 4343528 into jaegertracing:master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Collector: Sequence of shutdown calls resulting in crash due to panic and leads to span loss

2 participants