refactor: remove trivial DEFAULT_CONFIG from instrumentation

[blumamir]

This PR applies some cleanups regarding the use of DEFAULT_CONFIG in 5 instrumentations.

It only targets cases where the unset (undefined) values are already the "safe" default values, and adding a DEFAULT_CONFIG adds no real value.

There are other places where DEFAULT_CONFIG is used such that the default values are numbers, or true, thus they are not trivial and need more considerations.

These are:

amqplib:

export const DEFAULT_CONFIG: AmqplibInstrumentationConfig = {
  consumeTimeoutMs: 1000 * 60, // 1 minute
};

runtime-node:

const DEFAULT_CONFIG: RuntimeNodeInstrumentationConfig = {
  eventLoopUtilizationMeasurementInterval: 5000,
};

ioredis:

const DEFAULT_CONFIG: IORedisInstrumentationConfig = {
  requireParentSpan: true,
};

knex:

const DEFAULT_CONFIG: types.KnexInstrumentationConfig = {
  maxQueryLength: 1022,
};

This PR is not breaking any public API or behavior and is internal refactor only.

[blumamir]

I made sure to check every usage of such an property and verified it will handle undefined as the current default value (false in most cases, by checking for Truthy values which are falsy for both undefiend and false)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.38%. Comparing base (dfb2dff) to head (c631688).
Report is 135 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2217      +/-   ##
==========================================
- Coverage   90.97%   90.38%   -0.60%     
==========================================
  Files         146      147       +1     
  Lines        7492     7497       +5     
  Branches     1502     1571      +69     
==========================================
- Hits         6816     6776      -40     
- Misses        676      721      +45     

Files	Coverage Δ
...etry-instrumentation-bunyan/src/instrumentation.ts	97.87% <100.00%> (-0.83%)	⬇️
...try-instrumentation-graphql/src/instrumentation.ts	92.52% <100.00%> (-0.29%)	⬇️
...opentelemetry-instrumentation-graphql/src/utils.ts	93.65% <100.00%> (+0.48%)	⬆️
...y-instrumentation-memcached/src/instrumentation.ts	96.15% <100.00%> (-0.28%)	⬇️
...try-instrumentation-redis-4/src/instrumentation.ts	81.21% <100.00%> (+1.12%)	⬆️
...metry-instrumentation-redis/src/instrumentation.ts	92.50% <100.00%> (-0.98%)	⬇️

... and 39 files with indirect coverage changes

[trentm]

One change here (and in the constructor above) is that the given config object is no longer a copy. A subsequent change to that passed in config object could unexpectedly have an impact here.

[blumamir]

Nice catch. Fixed (in memcahed as well).

The constructor does not need to shallow copy the config, as it is already done in the constructor of InstrumentationBase

[trentm]

There is some value in being explicit about the defaults. Instead of it clearly being a boolean value, the maintainer of the code needs to think about handling the undefined value. That's reasonably easy for booleans. However, is the same true for the depth: -1 in instr-graphql? Is if (config.depth >= 0 && config.depth < depth) immediately obvious to you when config.depth is undefined?

I guess arguably the code already should have been handling undefined values given the Config types all define the config vars as optional. Hrm.

In the Apache voting sense, I'm -0 on this change. I.e., I won't block this, but I'm not convinced of the value of the change.

[blumamir]

There is some value in being explicit about the defaults. Instead of it clearly being a boolean value, the maintainer of the code needs to think about handling the undefined value.

I think it's a style decision that we need to take and apply to all instrumentations. Should we explicitly state the defaults even in the trivial case (assuming undefined is used as a Truty comparison that treats it as false. Or should we omit the default values when trivial. I think we should keep the code lean and clean and avoid unnecessary complexity when possible. But I am open to changing that if there is a preference to be explicit about default values.

That's reasonably easy for booleans. However, is the same true for the depth: -1 in instr-graphql? Is if (config.depth >= 0 && config.depth < depth) immediately obvious to you when config.depth is undefined

The way I see it, is that the current code that says (config.depth >= 0 && config.depth < depth) allows itself not to check for undefined since we already made it a number with DEFAULT_CONFIG. While it works, it's not immediate from reading this line why we do not check the undefined case, and I think it's cleaner and more readable to verify empty values once when used instead of splitting the logic into 2 dependent parts.

This practice is also not type-safe (I think) as we access a property of type number | undefined and treat if as a number.

In the Apache voting sense, I'm -0 on this change. I.e., I won't block this, but I'm not convinced of the value of the change.

I hope the arguments above helped you be more convinced. I am happy to discuss any code style alternative and decide on the best convention, and then make all instrumentations consistent with the agreed-upon style.

[trentm]

If I can have a pony :) what I'd want as an instrumentation maintainer is this._config (or .getConfig() or whatever API) to give me a config object that has a known type and valid value for every config var. In "type-speak", I'd want separate types for BunyanInstrumentationConfig (with disableLogSending: boolean) and BunyanInstrumentationConstructorOptions (with disableLogSending?: boolean, i.e. can be undefined).

At least that is the mental model I had when using DEFAULT_CONFIG. It means that config is resolved at the API points where config can be provided, and the rest of the code does not need to worry about invalid values. In systems with more complex config, doing all the validation at these API points is the lean and clean path.

Here invalid could mean more than an undefined or empty value. E.g. a JavaScript user of instrumentation-bunyan could pass in new BunyanInstrumentation({disableLogSending: "yes"}).

However, I'm not advocating doing all that now. I expect instrumentation config code to have to adapt a bit for coming file-config work.

[blumamir]

If I can have a pony :) what I'd want as an instrumentation maintainer is this._config (or .getConfig() or whatever API) to give me a config object that has a known type and valid value for every config var. In "type-speak", I'd want separate types for BunyanInstrumentationConfig (with disableLogSending: boolean) and BunyanInstrumentationConstructorOptions (with disableLogSending?: boolean, i.e. can be undefined).

At least that is the mental model I had when using DEFAULT_CONFIG. It means that config is resolved at the API points where config can be provided, and the rest of the code does not need to worry about invalid values. In systems with more complex config, doing all the validation at these API points is the lean and clean path.

I appreciate and support your suggestions for enhancing how to manage configurations in our instrumentations, and think this can be awesome to have. Would love to contribute to this initiative, but my current goal with this PR is to eliminate redundant config values, aligning it with the practices of most other instrumentations.

In the future, we could certainly explore a better approach, such as differentiating between user-supplied and "resolved" default configurations, having 2 types, or reducing redundancy by resolving configurations in the base class. This can be added regardless of this change at anytime.

Here invalid could mean more than an undefined or empty value. E.g. a JavaScript user of instrumentation-bunyan could pass in new BunyanInstrumentation({disableLogSending: "yes"}).

This can spawn a whole different discussion, on what values do we treat as turthy and then make sure we use this pattern everywhere. We can address such cases potentially in a different PR that target these concerns. I personally think that we should stick with the boolean type, until the time we encounter a practical need. at least for booleans (as you can see I left controversial cases out of this PR and only addressed trivial cases.

However, I'm not advocating doing all that now. I expect instrumentation config code to have to adapt a bit for coming file-config work.

If it's ok with you, I propose we continue with this PR as it stands, since it already enhances the consistency of our instrumentation styles. If you have the time and motivation, I encourage you to develop a more comprehensive solution.

[trentm]

This can spawn a whole different discussion, on what values do we treat as turthy

My using "yes" was a bad example. I did not mean to suggest it as a possible acceptable value for a boolean config var. Rather I was trying to show an example of a bogus invalid argument value given by the user.

I propose we continue with this PR as it stands

Okay. I disagree, but I don't object strongly.