Skip to content
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

fix: fix restify instrumentation utils that might accept undefined types #690

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

faizhasim
Copy link
Contributor

Which problem is this PR solving?

  • Using restify instrumentation on middlewares that return nothing (return undefined) will break the instrumentation due to typeof undefined.then.

Short description of the changes

  • This PR just add a simple defensive check on possibly undefined value.
  • Additional coverage added for utils.ts

@faizhasim faizhasim requested a review from a team October 6, 2021 10:28
@github-actions github-actions bot requested a review from rauno56 October 6, 2021 10:28
@faizhasim faizhasim force-pushed the fix-possibly-null-is-promise branch from 071873b to 12d2873 Compare October 6, 2021 10:38
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #690 (f625f8c) into main (aeadca8) will decrease coverage by 0.20%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
- Coverage   96.82%   96.62%   -0.21%     
==========================================
  Files           9       16       +7     
  Lines         630     1036     +406     
  Branches      124      151      +27     
==========================================
+ Hits          610     1001     +391     
- Misses         20       35      +15     
Impacted Files Coverage Δ
...opentelemetry-instrumentation-restify/src/types.ts 100.00% <0.00%> (ø)
...nstrumentation-restify/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
...try-instrumentation-restify/src/instrumentation.ts 89.32% <0.00%> (ø)
...lemetry-instrumentation-restify/test/utils.test.ts 89.28% <0.00%> (ø)
...opentelemetry-instrumentation-restify/src/utils.ts 100.00% <0.00%> (ø)
...metry-instrumentation-restify/test/restify.test.ts 99.60% <0.00%> (ø)
...telemetry-instrumentation-restify/src/constants.ts 100.00% <0.00%> (ø)

@faizhasim faizhasim requested a review from Flarna October 6, 2021 13:56
@faizhasim faizhasim requested a review from Flarna October 6, 2021 14:14
@rauno56
Copy link
Member

rauno56 commented Oct 12, 2021

👍 Good catch. I don't see a reason to install and use Bluebird though for the tests.

@faizhasim
Copy link
Contributor Author

👍 Good catch. I don't see a reason to install and use Bluebird though for the tests.

I added that because #690 (comment). I would incline towards not including non built-in promise. I think, by now, it should be the responsibility of the custom promise author to write it according to the promise a+ spec. It might be over-stretching to also test custom promise or popular promise implementation like bluebird/rsvp/etc.

Regardless, advice me. I can modify this PR.

@rauno56
Copy link
Member

rauno56 commented Oct 12, 2021

Yeah. Really sorry to bounce you back and forth like that, but I don't think it's worth the extra dep.
As the component owner, I'd be happy to merge, once that's removed and the conflicts resolved.

@faizhasim faizhasim force-pushed the fix-possibly-null-is-promise branch from 8f83e02 to ec160ef Compare October 12, 2021 10:58
@faizhasim faizhasim force-pushed the fix-possibly-null-is-promise branch from ec160ef to 7925fab Compare October 12, 2021 11:03
Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Flarna
Copy link
Member

Flarna commented Oct 12, 2021

Instead of bluebird or some other promise 3rd party you could write your own promise like object in a test.
My main point is that the instrumentation explicit tests on typeof(foo.then) === "function" and not for instanceof Promise therefore I assumed this should be verified.

But I fine with removing bluebird.

@rauno56
Copy link
Member

rauno56 commented Oct 12, 2021

That makes sense, yes. The currently implemented version of the util is a bit more restrictive also checking for toString which is commonly overwritten by the promise libs - bluebird and Q do it at least. Haven't tested others.

Let's leave it for now - the current PR is solving an issue it intended to.

@rauno56 rauno56 changed the title fix: Fix restify instrumentation utils that might accept undefined types fix: fix restify instrumentation utils that might accept undefined types Oct 12, 2021
@rauno56 rauno56 merged commit bc11f3d into open-telemetry:main Oct 12, 2021
@faizhasim faizhasim deleted the fix-possibly-null-is-promise branch October 12, 2021 22:02
@dyladan dyladan mentioned this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants