fix(prometheus): metrics.ts leaks to global registry #1202 #1203
Conversation
This file contains 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
…i#1202 1. Specified a `register` property of the gauges as an empty array so that it does not pollute the global namespace. This is how this is supposed to be done as per the docs of prom-client. 2. Once the change from 1) took place, the issue became that the metrics gathering code was still trying to hit up the global scope for the metrics, e.g. calling the get metrics methods directly on the promClient object instead of the registry that we create for each prmoetheus exporter object separately. So a little additional refactor ensued to fix this as well by making sure that we grab a reference of the registry object at construction time and then re-use that wherever needed instead of going through the global promClient object. 3. Added missing .startMetricsCollection calls in the plugin constructors to ensure that the prometheus exporter object gets initialized properly (this is where the registry gets created as well so without this there are crashes happening when one tries to access the metrics through the registry) Why though? The problem was that the metrics.ts file that we have for all the plugin's metrics constructs a new Metric (Gauge) object at import time which then defaults to registering the metric in the global registry of prom-client by default. The latter was causing crashes when separate versions of the same metrics.ts file are imported in a scenario were the API server imports plugins from a different directory (this issue is coming from the branch where I'm working on plugin sandboxing via the live-plugin-manager). Fixes hyperledger-cacti#1202 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Aug 10, 2021
…er-cacti#1192 Migrates to the live-plugin-manager package to install plugins instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The hope with the new live-plugin-manager package is that using this will provide sufficient isolation so that these kind of issues are non-existent and also that it does not introduce other different types of issues stemming from exactly said isolation. With that said if isolation problems do occur we'll have to fix that anyway because the plugins should not depend on the API server and vica versa. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
Codecov Report
@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
+ Coverage 71.76% 71.95% +0.19%
==========================================
Files 270 270
Lines 9420 9431 +11
Branches 1113 1113
==========================================
+ Hits 6760 6786 +26
+ Misses 2087 2072 -15
Partials 573 573
Continue to review full report at Codecov.
|
|
LGTM |
takeutak
approved these changes
Aug 11, 2021
petermetz
added a commit
to petermetz/cacti
that referenced
this pull request
Aug 12, 2021
…er-cacti#1192 Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
petermetz
added a commit
that referenced
this pull request
Aug 13, 2021
Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes #1192 Depends on #1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
hanxu12
pushed a commit
to hanxu12/cactus
that referenced
this pull request
Aug 14, 2021
…er-cacti#1192 Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com> Signed-off-by: hxlaf <xuhan@lafayette.edu>
maramih
pushed a commit
to maramih/cactus
that referenced
this pull request
Aug 17, 2021
…er-cacti#1192 Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
Leeyoungone
pushed a commit
to Leeyoungone/cactus
that referenced
this pull request
Aug 27, 2021
…er-cacti#1192 Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
elenaizaguirre
pushed a commit
to elenaizaguirre/cactus
that referenced
this pull request
Sep 20, 2021
…er-cacti#1192 Migrates to the live-plugin-manager package to install plugins instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The hope with the new live-plugin-manager package is that using this will provide sufficient isolation so that these kind of issues are non-existent and also that it does not introduce other different types of issues stemming from exactly said isolation. With that said if isolation problems do occur we'll have to fix that anyway because the plugins should not depend on the API server and vica versa. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
RafaelAPB
pushed a commit
to RafaelAPB/blockchain-integration-framework
that referenced
this pull request
Mar 9, 2022
…er-cacti#1192 Migrates to the lmify package to install plugins at runtime instead of doing it via vanilla npm which was causing problems with conflicting dependency versions where the API server would want semver 7.x and one of the plugins (through some transient dependency of the plugin itself) would install semver 5.x which would then cause the API server to break down at runtime due to the breaking changes between semver 7 and 5. The magic sauce is the --prefix option of npm which, when specified instructs npm to ignore the usual parent directory traversal algorithm when evaluating/resolving dependency trees and instead just do a full installation to the specified directory path as dictated by the --prefix option. This means that we can install each plugin in their own directory the code being isolated from the API server and also from other plugins that might also interfere. Fixes hyperledger-cacti#1192 Depends on hyperledger-cacti#1203 Signed-off-by: Peter Somogyvari <peter.somogyvari@accenture.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fix(prometheus): metrics.ts leaks to global registry #1202
Specified a
registerproperty of the gauges as an emptyarray so that it does not pollute the global namespace. This
is how this is supposed to be done as per the docs of prom-client.
Once the change from 1) took place, the issue became that
the metrics gathering code was still trying to hit up the
global scope for the metrics, e.g. calling the get metrics
methods directly on the promClient object instead of the
registry that we create for each prmoetheus exporter object
separately. So a little additional refactor ensued to fix this
as well by making sure that we grab a reference of the registry
object at construction time and then re-use that wherever needed
instead of going through the global promClient object.
Added missing .startMetricsCollection calls in the plugin
constructors to ensure that the prometheus exporter object
gets initialized properly (this is where the registry gets
created as well so without this there are crashes happening
when one tries to access the metrics through the registry)
Why though?
The problem was that the metrics.ts file that we have for all the
plugin's metrics constructs a new Metric (Gauge) object at import
time which then defaults to registering the metric in the global
registry of prom-client by default.
The latter was causing crashes when separate versions of the same
metrics.ts file are imported in a scenario were the API server
imports plugins from a different directory (this issue is coming
from the branch where I'm working on plugin sandboxing via the
live-plugin-manager).
Fixes #1202
Signed-off-by: Peter Somogyvari peter.somogyvari@accenture.com