Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Graceful Shutdown Support #1321
Add Graceful Shutdown Support #1321
Changes from 66 commits
871d696
da633ad
de161cc
edd3b49
16c0faf
f0b9ef6
4bd85b5
86cc4e9
5d93f6f
9ad87e5
1d3ea53
0a99520
791156b
7d714b6
b106b50
ba5be87
064c8e8
54a82c5
8fd53c8
b5af380
e159546
c0d3f5f
9ceba0e
078802d
adedac1
44a7275
a1bfb12
3dd51d2
d541837
0163241
95bb8bf
ef84611
6f4de1e
5597957
7c09806
b956e9c
ab0de32
c4ffad7
32db1a9
db4745e
0a0a487
802c5ad
e6d49b4
5727e6e
451c99c
9336bfa
bf46c1b
62966b5
d07163a
f0323e6
4d92d39
70bee58
ef046b9
5b14fb5
73edc9a
12b18fd
b0fdffd
81f7dee
93719b9
a0d309d
a28cfb0
9c21dbb
bfa1a0a
ec3ce12
49e1a69
8afa9a1
32439ab
529df9b
d5c430c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is
this._config.exporter.shutdown()
actually blocking, or is this a missing callback or missing async decoration?It may be out of scope to fix, but an issue could be created to look into this.
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.
I don't think exporter shutdown calls are async. Regardless I'm not sure if it matters if this is blocking?
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.
I think the exporters don't do any batching so it should be ok to have the shutdown be sync. On the other hand, it may be worth adding to the API just to allow the flexibility in the future.
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.
Technically, the exporters could batch here: Say you have 1K unflushed spans, and the backend API supports 100 spans per batch call with a 500 ms limit between calls. -> shutdown would take 5 seconds.
I'm not sure it matters either. Perhaps something to potentially address in a future API design sprint.
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.
this is fine, but you can remove async now from the function
_shutdownAllMeters
as nothing is async anymore here