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.
Add Graceful Shutdown Support #1321
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
Changes from 56 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.
"export" function from exporters return "void" there is no reason to call "await" in front of it as it will not block anything.
IF you really want to wait for exporter to finish you should create here a promise and resolve or reject it in a callback where you added "@todo
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.
The todo was actually there before my time, I removed this await - you make a good point about it not being async.
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.
but you can return a promise to be resolved when exporter exports something (or rejected depends on callback) so this way the
_collect
will really wait until the exporter finishes - what was the original plan I believeThere 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.
It would be nice to highlight the pitfall of disabling the
gracefulShutdown
option.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 added a little more emphasis about why it's good to enable graceful shutdown.
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.
Since
notifyOnGlobalShutdown()
returns a shutdown function, is there a reason why you don't use that on here?This applies to all tests
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 option would require us to modify the trace/meterprovider to store the cleanup function, and then modify every single test case which uses a trace/meter provider to store and then call the cleanup function. I initially tried this but the task is extremely daunting and would require all future tests to do this as well.
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.
Fair enough. I guess this is pretty much what @obecny tried to point at in https://github.com/jonahrosenblum/opentelemetry-js/pull/2/files
I don't have a strong opinion on where this should go, or if it needs to be addressed in this PR. I'm fine with this for now.
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 approach does not remove the listeners from every other unit test which uses a trace/meterprovider and as a result will still have test cases fail.
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 not failing -> #1413
The only failing test was
InMemorySpanExporter.test.ts
due to one initialisation instead of before each test.I have also added the missing "cleanup" if for any reason the shutdown will be called directly - you should also remove events that were attached to avoid memory leaking.
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 makes sense, I have merged your changes. Thank you for your patience I was not fully understanding your implementation until now.