Verify extension lifecycle #2679
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a test to ensure all extension types can be created, started, and stopped multiple times. Fix the issues found with the test on the extensions.
Codecov Report
@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
+ Coverage 91.73% 91.79% +0.05%
==========================================
Files 290 290
Lines 15614 15633 +19
==========================================
+ Hits 14324 14350 +26
+ Misses 892 886 -6
+ Partials 398 397 -1
Continue to review full report at Codecov.
|
tigrannajaryan
approved these changes
Mar 11, 2021
bogdandrutu
reviewed
Mar 12, 2021
bogdandrutu
reviewed
Mar 12, 2021
Comment on lines
-57
to
-64
| // The runtime settings are global to the application, so while in principle it | ||
| // is possible to have more than one instance, running multiple does not bring | ||
| // any value to the service. | ||
| // In order to avoid this issue we will allow the creation of a single | ||
| // instance once per process while keeping the private function that allow | ||
| // the creation of multiple instances for unit tests. Summary: only a single | ||
| // instance can be created via the factory. | ||
| if !atomic.CompareAndSwapInt32(&instanceState, instanceNotCreated, instanceCreated) { |
There was a problem hiding this comment.
I think this comment was explaining nicely the motivation, why do you need to do it differently?
There was a problem hiding this comment.
The comment applies to pprof since it is the one that has runtime settings for the whole process. The comments for pprof were moved to the start method.
I can't repro this locally so far, I will try some variations but may have to resort to attempting merges without certainty about the CI issue. |
bogdandrutu
reviewed
Mar 13, 2021
bogdandrutu
reviewed
Mar 13, 2021
bogdandrutu
reviewed
Mar 16, 2021
|
@bogdandrutu are we good to merge this? |
bogdandrutu
approved these changes
Mar 17, 2021
hughesjj
pushed a commit
to hughesjj/opentelemetry-collector
that referenced
this pull request
Apr 27, 2023
Troels51
pushed a commit
to Troels51/opentelemetry-collector
that referenced
this pull request
Jul 5, 2024
--------- Co-authored-by: Marc Alff <[email protected]> Co-authored-by: Lalit Kumar Bhasin <[email protected]> Co-authored-by: Tom Tan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Why
The lifetime of extensions shouldn't be tied to single creation and start. It should be possible to create multiple instances even if they are limited to having only one active instance. This allows changing the extensions without restarting the process.
What
Description:
Ensure that extensions can be created and started multiple times. This change adds a test to ensure all extension types can be created, started, and stopped multiple times and fixes the issues found during the test of the default extensions.
healthcheckextensionandzpagesextension) were being limited to be created only once per process. The artificial limitation was removed;pprofextensionshould only have one extension active since its settings are global to the process. The extension now controls for active instances, not the creation of instances, since it is more appropriate.Testing: