-
Notifications
You must be signed in to change notification settings - Fork 816
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
feat: merge user supplied and default plugin configs #980
feat: merge user supplied and default plugin configs #980
Conversation
Can you please add tests |
8402db4
to
32dc2c8
Compare
434d742
to
19bd6c3
Compare
Codecov Report
@@ Coverage Diff @@
## master #980 +/- ##
==========================================
+ Coverage 92.34% 92.36% +0.02%
==========================================
Files 115 115
Lines 3398 3407 +9
Branches 684 686 +2
==========================================
+ Hits 3138 3147 +9
Misses 260 260
|
19bd6c3
to
1c1265d
Compare
} else { | ||
mergedUserSuppliedPlugins[pluginName] = config.plugins[pluginName]; | ||
// enable user-supplied plugins unless explicitly disabled | ||
if (mergedUserSuppliedPlugins[pluginName].enabled === undefined) { |
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 enabled
a required plugin config? If not, I think it would be a good field to add.
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.
Seems like it is not required:
opentelemetry-js/packages/opentelemetry-api/src/trace/instrumentation/Plugin.ts
Lines 56 to 60 in b1fe2c8
/** | |
* Whether to enable the plugin. | |
* @default true | |
*/ | |
enabled?: boolean; |
But it also seems to default to true
but I'm not sure why/how that gets lost. That's why I am making user supplied plugins (that are not part of the default plugin list) enabled
parameter default to true
here.
b80a7bc
to
f4aeb6e
Compare
bump |
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 we should add a specific test that assert that the default options are not overriden by the user one to be sure we dont introduce a regression in the future
Default options should be overridden by user options. |
@dyladan Sorry i meant the fact that the user pass its custom options override all default options |
@naseemkullah sorry this has been such a long process. I agree with @vmarchaud we should probably have a test that if user supplies config for |
np thanks for the feedback @dyladan @vmarchaud , I'll work on getting the test and let you know if any blockers come up (fetching the plugin config and being able to check it in a test for |
By merging user supplied config two levels deep, a user can now only configure the diff from the default auto enablement of installed plugins, rather than having to explicitly re enable all plugins if at least one of their configurations require are edited. Furthermore, user supplied plugins not listed in default plugins are implicitly enabled. Signed-off-by: Naseem <naseem@transit.app>
ba94fae
to
d6a0902
Compare
Alright I added a test by checking if grpc plugin would successfully load without any grpc related config in a situation with user supplied plugin config. Furthermore, not sure how to incorporate this in test, but i console logged
|
I am having issues running lint and lint:fix locally, might have something to do with recent migration to eslint, any ideas @dyladan @mayurkale22 ?
In any case i will adjust lint errors manually. This should be ready to go upon build success. |
Signed-off-by: Naseem <naseem@transit.app>
I would first try to do a full clean and rebuild as the dependencies are all very different lerna clean -y && rm -rf node_modules && npm install |
tried from the |
This should be run from the root folder. If you're on windows you may need to split it into separate commands:
|
Ah ok, so from root folder should work. I've been living Windows free since my last enterprise gig a few years back! |
Seem to be getting errors on non related code:
|
Those are warnings not errors that you posted. The warnings don't fail the build |
#1093 is created to address these warnings. Please ignore it for now. |
Ah right but still seems to be the case, message updated to include more context |
There are errors as well, |
hmm. the question is why does it not break the build for others... |
$ git clean -dfx
$ npm install
$ npm run lint I tried these commands locally and everything is working well for me. |
now
|
Should be fixed with #1094 |
I think it is most likely a versioning discrepancy as the lint versions are not pinned in CI. #1081 also fixes this shortcoming. |
Trying to confirm that the regression in the plugin load behaviour is intentional or not. My intention is to have a few plugins loaded for instrumentation. I have been loading an array of const plugins = composePlugins(options.plugins || defaultPlugins, options.pluginOptions || {});
const config = {
plugins,
sampler: ALWAYS_SAMPLER,
traceParams: {
numberOfEventsPerSpan: 64,
numberOfAttributesPerSpan: 64
},
logger
};
this.#provider = new NodeTracerProvider(config); We are intentionally not instrumenting some other libraries, for example, With this change, we are getting errors on startup:
I see now that all the I would like to remove all of these default plugins, or disable them all, but I cannot see an exposed way to do that. 3 questions:
For the moment, I am doing this: // See https://github.com/open-telemetry/opentelemetry-js/pull/980
const DEFAULT_DISABLE_HACK = {
mongodb: { enabled: false },
grpc: { enabled: false },
http: { enabled: false },
https: { enabled: false },
mysql: { enabled: false },
pg: { enabled: false },
redis: { enabled: false },
ioredis: { enabled: false },
'pg-pool': { enabled: false },
express: { enabled: false }
};
.....
const useplugins = composePlugins(options.plugins || defaultPlugins, options.pluginOptions || {});
const plugins = { ...DEFAULT_DISABLE_HACK, ...useplugins };
const config = {
plugins,
sampler: ALWAYS_SAMPLER,
traceParams: {
numberOfEventsPerSpan: 256,
numberOfAttributesPerSpan: 256
},
logger
};
this.#provider = new NodeTracerProvider(config); |
Hi @rlibm
to something like
Or add yet another config option equivalent to your @dyladan @mayurkale22 thoughts comments or corrections to the above? |
The specific issue I have is this code is being put in a library that wraps opentelemetry, and fortunately for me, I noticed the error before it went further. I am fortunate that the first system I tested it on had ioredis installed, and threw that error, and I am also fortunate that the Otherwise the ioredis and other plugins may have been silently, and unexpectedly loaded, and caused significant increases in tracing output, and possibly issues in down-stream automated anomoly detection, etc. In effect, the risk I have is 2-fold:
I would recommend some option on the NodeTracingProvider that bypasses the merge, and uses the supplied plugins as-configured. Automatic merging with unknown content is a bad solution in enterprise environments. This includes not just completely new plugins, but also options on existing plugins. Changing the options, or adding new options on default plugins could cause significant regressions in existing applicatoins when new (even minor) versions of OpenTelemetry are released. |
Is it fortune to not have In any case, I think a |
We have the desire to not instrument ioredis. for cloud.ibm.com the redis volumes are huge. Inadvertently enabling it would be .... problematic. Side-Note, we're migrating from OpenCensus to OpenTelemetry. The OpenTelemetry is really close to production (yes, we know this library is not yet out of Beta). I will work on a PR, but we natively use JavaScript, I don't have tooling set up for .ts as such. May take a while. |
@rlibm Could you elaborate on the downsides of instrumenting a huge redis dataset? Do you think it would add latency and if so would it be possible to do some benchmarking? If it's a question of data produced, there are sampling strategies to use, also FYI this PR prevents random background jobs from being traced, and only redis ops that have a parent span will be traced. That is exciting that IBM Cloud is migrating to OTEL! 🚀 FWIW We are starting to use this quite a bit in production at Transit, but we are a startup 😄 Great to hear you are up for a PR! 👍 TS is great, you're gonna love it... and no rush, we'll be waiting! 😄 |
ooh and one more thing, don't be a stranger if you encounter any time wasters/blockers when working on the PR https://gitter.im/open-telemetry/opentelemetry-node |
fixes #616
Which problem is this PR solving?
By merging user supplied config two levels deep, a user can now only configure the diff
from the default auto enablement of installed plugins, rather than
having to explicitly re enable all plugins if at least one of their
configurations require are edited.
Furthermore, user supplied plugins not listed in default plugins are
implicitly enabled.
Short description of the changes
User supplied config is checked for any default plugin configs, those are merged with the default config of default plugins.
User supplied plugins which are not part of the default plugins list are implicitly enabled, but can be disabled with
enabled: false
property.The merged user supplied plugins are then merged with the default plugins to include any default plugins that may not have been configured by the user.