chore: Fix lint warnings in instrumentation package by alisabzevari · Pull Request #2404 · open-telemetry/opentelemetry-js

alisabzevari · 2021-08-08T17:31:27Z

related to #1093,

Which problem is this PR solving?

This PR fixes lint warnings in instrumentation package.

Short description of the changes

Fix lint warnings
Note all warnings are resolved. The following warnings are still unresolved since they need better understanding about the code and its usage. I hope I can fix them later when I gain more knowledge about the project.

/home/asabzevari/projects/opentelemetry/opentelemetry-js/packages/opentelemetry-instrumentation/src/instrumentation.ts
  32:51  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

/home/asabzevari/projects/opentelemetry/opentelemetry-js/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
  28:47  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

/home/asabzevari/projects/opentelemetry/opentelemetry-js/packages/opentelemetry-instrumentation/src/platform/node/instrumentationNodeModuleDefinition.ts
  30:39  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

codecov · 2021-08-08T18:34:38Z

Codecov Report

Merging #2404 (bddeafb) into main (e8e787d) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2404      +/-   ##
==========================================
+ Coverage   93.03%   93.04%   +0.01%     
==========================================
  Files         137      138       +1     
  Lines        5069     5093      +24     
  Branches     1089     1096       +7     
==========================================
+ Hits         4716     4739      +23     
- Misses        353      354       +1

Impacted Files	Coverage Δ
...entelemetry-instrumentation/src/autoLoaderUtils.ts	`92.00% <0.00%> (ø)`
...telemetry-sdk-trace-node/src/NodeTracerProvider.ts	`95.83% <0.00%> (ø)`

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

MSNev · 2021-08-10T02:47:33Z

packages/opentelemetry-instrumentation/src/autoLoaderUtils.ts

      const results = parseInstrumentationOptions(option);
      instrumentations = instrumentations.concat(results.instrumentations);
-    } else if (typeof option === 'function') {
+    } else if (isInstrumentationClass(option)) {


nit: this and line 39 seem like overkill, especially as isInstrumentationClass is just checking if option is a function and not specifically that is a class / constructor. Especially this this really just causes more code to be generated than necessary (coming from a code / browser support minification perspective)

Oh, and don't appear to be related to the listed linting issues.

Replacing this function with typeof option === 'function' results in the following compile error:

Cannot create an instance of an abstract class.ts(2511)

The nice thing about this type guard function is that it implicitly casts the option to the desired type as well.

The isInstrumentation function can be removed though. I just added it to improve the readability of the code (by my personal standards!).

That would be a side effect that the function is declaring that the object is of type option is new () => T (which could be added inline) as this gets compiled away.

So simplistically, you have a function that is kludging this. If you want a function I would suggest that we create this as a helper and call it something like isConstructor although that would still just check that it's a function.

... declaring that the object is of type option is new () => T (which could be added inline) ...

That's actually a good idea. Can you tell me how to do it? I cannot figure out the syntax to do it in the if condition.

It would require a change, but it could be done. The meta package could easily return a list of already new'd instrumentations. The constructor itself doesn't do any patching or have side effects.

it's not pretty but something like this should work, hence my suggestion of a generic isConstructor helper

} else if (typeof option === 'function') { instrumentations.push(new (option as unknown as (new () => InstrumentationOption))()); }

I prefer disabling the eslint warning instead of cast to unknown then to something else which only pretends to be typesafe. If the call to new fails, then the user will get an error during process start. We can't guard against every possible case.

What is the resolution here?

It seems like the current solution is not preferrable. I will change it back to use any and disable the eslint for that line.

obecny · 2021-08-10T19:48:19Z

packages/opentelemetry-instrumentation/src/autoLoaderUtils.ts

 ): AutoLoaderResult {
  let instrumentations: Instrumentation[] = [];
  for (let i = 0, j = options.length; i < j; i++) {
-    const option = options[i] as any;


actually any is valid case here. This will be coming from a user so it can be written in ts or pure js, I would not change that.

If any is valid here, then the InstrumentationOption type should be changed?
Or the function parameter type should be any[] or unknown[].

He is simply saying it is possible for the function to be called with bad inputs and saying you should guard against that. Unlike a real staticly typed language, we can always receive bad input. IMO failing during setup if bad inputs are passed is fine, but if something is easy to guard against I see no reason not to.

obecny · 2021-08-10T19:52:10Z

packages/opentelemetry-instrumentation/src/autoLoaderUtils.ts

      const results = parseInstrumentationOptions(option);
      instrumentations = instrumentations.concat(results.instrumentations);
-    } else if (typeof option === 'function') {
+    } else if (isInstrumentationClass(option)) {


I still think we should only allow Instrumentation[] but that would need to be handled in a separate PR since it is breaking and quite a substantial change.

not really, the meta package will return array of instrumentations, and you still want to be able to do that,

[ getNodeAutoInstrumentations(), new MyOwnPackage()]

packages/opentelemetry-instrumentation/src/platform/node/types.ts

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

packages/opentelemetry-instrumentation/src/autoLoaderUtils.ts

vmarchaud · 2021-09-15T08:41:24Z

cc @alisabzevari could you rebase your PR ? there are conflicts :/

…ntationOptions

alisabzevari · 2021-11-10T18:43:43Z

Done @vmarchaud, please have a look.

alisabzevari requested a review from a team August 8, 2021 17:31

alisabzevari force-pushed the lint-warnings-instrumentation branch from 872604d to 97234e4 Compare August 8, 2021 18:29

dyladan reviewed Aug 9, 2021

View reviewed changes

packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts Outdated Show resolved Hide resolved

dyladan approved these changes Aug 9, 2021

View reviewed changes

MSNev reviewed Aug 10, 2021

View reviewed changes

alisabzevari changed the title ~~Fix lint warnings in instrumentation package~~ chore: Fix lint warnings in instrumentation package Aug 10, 2021

obecny reviewed Aug 10, 2021

View reviewed changes

dyladan self-requested a review August 10, 2021 20:19

vmarchaud requested review from MSNev and obecny September 6, 2021 19:05

obecny approved these changes Sep 7, 2021

View reviewed changes

johnbley approved these changes Sep 8, 2021

View reviewed changes

MSNev approved these changes Sep 10, 2021

View reviewed changes

dyladan added the internal label Sep 15, 2021

alisabzevari added 4 commits November 10, 2021 19:38

chore(instrumentation): fix lint warnings

8823214

chore(instrumentation): fix type parameter of InstrumentationModuleFile

d9412d8

chore(instrumentation): rename type parameter

8e0f46c

chore(instrumentation): disable eslint for type cast in parseInstrume…

9045870

…ntationOptions

alisabzevari force-pushed the lint-warnings-instrumentation branch from 2cdb763 to 9045870 Compare November 10, 2021 18:42

chore(instrumentation): fix return type name

bddeafb

dyladan merged commit fdab642 into open-telemetry:main Nov 10, 2021

dyladan mentioned this pull request Nov 10, 2021

chore: release proposal v1.0.1 / v0.27.0 #2611

Merged

Conversation

alisabzevari commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which problem is this PR solving?

Short description of the changes

Uh oh!

codecov bot commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vmarchaud commented Sep 15, 2021

Uh oh!

alisabzevari commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alisabzevari commented Aug 8, 2021 •

edited

Loading

codecov bot commented Aug 8, 2021 •

edited

Loading