chore: fix lint errors on opentelemetry-context-async-hooks #2338

macabeus · 2021-07-08T17:56:52Z

Which problem is this PR solving?

Fix lint warnings #1093

Short description of the changes

Fix the following lint errors:

\opentelemetry-js\packages\opentelemetry-context-async-hooks\src\AbstractAsyncHooksContextManager.ts
   71:35  warning  Don't use `Function` as a type  @typescript-eslint/ban-types
  133:60  warning  Don't use `Function` as a type  @typescript-eslint/ban-types
  151:64  warning  Don't use `Function` as a type  @typescript-eslint/ban-types
  175:15  warning  Don't use `Function` as a type  @typescript-eslint/ban-types

\opentelemetry-js\packages\opentelemetry-context-async-hooks\src\AsyncHooksContextManager.ts
  49:22  warning  Forbidden non-null assertion  @typescript-eslint/no-non-null-assertion

linux-foundation-easycla · 2021-07-08T17:56:54Z

The committers are authorized under a signed CLA.

✅ Bruno Macabeus (e54869a)

macabeus · 2021-07-08T17:57:39Z

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

    });
    // patch methods that remove a listener
-    if (typeof ee.removeListener === 'function') {
-      ee.removeListener = this._patchRemoveListener(ee, ee.removeListener);
+    const { removeListener, off, removeAllListeners } = ee


Until we bump to TS 4.4, we'll need to do it.
https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-beta/#cfa-aliased-conditions

dyladan · 2021-07-08T18:57:56Z

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

@@ -18,6 +18,11 @@ import { ContextManager, Context } from '@opentelemetry/api';
 import { EventEmitter } from 'events';

 type Func<T> = (...args: unknown[]) => T;
+type UnknownFunc = Func<unknown>;


Does this type really save us anything?

It's just to avoid writing Func<unknown> on lines 24 and 76, and I think it's easier to read.
But I could remove this type if you prefer.

dyladan · 2021-07-08T19:00:57Z

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

@@ -62,13 +67,13 @@ export abstract class AbstractAsyncHooksContextManager
      return this._bindEventEmitter(context, target);
    }

-    if (typeof target === 'function') {
+    if (isUnknownFunc(target)) {


This fixes the lint error technically but doesn't actually fix the problem the lint is trying to point out. This will still return true for constructors so doesn't provide any additional safety. IMO it would be better to use a comment to disable the lint warning as a much more obvious solution.

There are some workarounds to check if something is a constructor: https://stackoverflow.com/questions/40922531/how-to-check-if-a-javascript-function-is-a-constructor
Perhaps it would better also checking in inUnknownFunc/isGenericFunc if the argument is a constructor and, if yes, returns false?
I would like to avoid a comment ignoring the lint error.

A comment ignoring the lint error clearly shows "i know this is dangerous and I accept it." A type which obfuscates the danger can come back to bite you later. If a constructor is passed to bind, there really isn't much we can or should do about it but let it fail.

edit: i suppose we could just skip and not bind it... @vmarchaud @obecny what do you think?

As far as I know NodeJs uses internally checks like typeof(arg) === 'function' and they don't care about constructors. Technically it's possible to create a function which can act as constructor and as normal function.

Therefore I don't think we should add a lot complexity here trying to fix a problem which is on caller side.

I agree with @Flarna here, i don't think we had the issue with any instrumentation so adding this much complexity for a problem that we don't have seems pointless right now. I would prefer to wait for an user having this issue to try to fix it

dyladan · 2021-07-08T19:01:36Z

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

-      if (ee[methodName] === undefined) return;
-      ee[methodName] = this._patchAddListener(ee, ee[methodName], context);
+      const method = ee[methodName];
+      if (isGenericFunc(method)) {


Same issue here, your type guard doesn't actually differentiate between this function and any other type of function.

Answered above.

codecov · 2021-07-08T21:06:13Z

Codecov Report

Merging #2338 (f8d9f25) into main (3a327dd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f8d9f25 differs from pull request most recent head e54869a. Consider uploading reports for the commit e54869a to get more accurate results

@@           Coverage Diff           @@
##             main    #2338   +/-   ##
=======================================
  Coverage   92.77%   92.77%           
=======================================
  Files         145      145           
  Lines        5216     5220    +4     
  Branches     1068     1068           
=======================================
+ Hits         4839     4843    +4     
  Misses        377      377

Impacted Files	Coverage Δ
...sync-hooks/src/AbstractAsyncHooksContextManager.ts	`97.18% <100.00%> (+0.16%)`	⬆️
...ontext-async-hooks/src/AsyncHooksContextManager.ts	`100.00% <100.00%> (ø)`

vmarchaud · 2021-07-11T07:36:30Z

packages/opentelemetry-context-async-hooks/src/AbstractAsyncHooksContextManager.ts

+type UnknownFuncReturns<T extends (...args: unknown[]) => unknown> = (...args: unknown[]) => ReturnType<T>;
+
+const isUnknownFunc = (fn: unknown): fn is UnknownFunc => typeof fn === 'function';
+const isGenericFunc = <T>(fn: (...args: never[]) => T): fn is Func<T> => typeof fn === 'function';


i don't see why we should differenciate between both, i think the second typeguard is fine to use everywhere no ?

dyladan · 2021-08-04T15:30:40Z

IMO this PR is doing more than just fixing lint errors. Adding function call overhead to such critical functions seems an unnecessary performance penalty to me just to remove lint errors.

github-actions · 2022-01-05T17:50:46Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions · 2022-01-24T06:40:34Z

This PR was closed because it has been stale for 14 days with no activity.

chore: fix lint errors on opentelemetry-context-async-hooks

e54869a

macabeus requested review from dyladan, Flarna, johnbley, legendecas, markwolff, mayurkale22, MSNev, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners July 8, 2021 17:56

macabeus commented Jul 8, 2021

View reviewed changes

dyladan reviewed Jul 8, 2021

View reviewed changes

vmarchaud reviewed Jul 11, 2021

View reviewed changes

vmarchaud added the internal label Jul 11, 2021

github-actions bot added the stale label Jan 5, 2022

dyladan closed this Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: fix lint errors on opentelemetry-context-async-hooks #2338

chore: fix lint errors on opentelemetry-context-async-hooks #2338

macabeus commented Jul 8, 2021

linux-foundation-easycla bot commented Jul 8, 2021 •

edited

Loading

macabeus Jul 8, 2021

dyladan Jul 8, 2021

macabeus Jul 8, 2021

dyladan Jul 8, 2021

macabeus Jul 8, 2021 •

edited

Loading

dyladan Jul 8, 2021 •

edited

Loading

Flarna Jul 9, 2021

vmarchaud Jul 11, 2021

dyladan Jul 8, 2021

macabeus Jul 8, 2021

codecov bot commented Jul 8, 2021 •

edited

Loading

vmarchaud Jul 11, 2021

dyladan commented Aug 4, 2021

github-actions bot commented Jan 5, 2022

github-actions bot commented Jan 24, 2022

chore: fix lint errors on opentelemetry-context-async-hooks #2338

chore: fix lint errors on opentelemetry-context-async-hooks #2338

Conversation

macabeus commented Jul 8, 2021

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macabeus Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

dyladan Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dyladan commented Aug 4, 2021

github-actions bot commented Jan 5, 2022

github-actions bot commented Jan 24, 2022

linux-foundation-easycla bot commented Jul 8, 2021 •

edited

Loading

macabeus Jul 8, 2021 •

edited

Loading

dyladan Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading