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

feat(restify): add requestHook support #1312

Merged
merged 11 commits into from
Dec 13, 2022

Conversation

tombruijn
Copy link
Contributor

The requestHook config option allows custom span handling per request layer.

Which problem is this PR solving?

  • Adds requestHook support so users can add custom handling on spans.

Short description of the changes

  • It's inspired by how ~requestHook` is implemented in web instrumentations.
  • The ~requestHookuser-defined function is called any time_handlerPatcher` is executed.

The `requestHook` config option allows custom span handling per request
layer.
@tombruijn tombruijn force-pushed the restify-request-hook branch from ff2957d to c66dbf9 Compare November 29, 2022 13:40
As mentioned in the review, pass the instrumentation config to the
parent class. That way the config is also stored when given to the
initializer, rather only when using the `setConfig` function.
Update comment to reference to correct type from the `@types/restify`
package.
@tombruijn tombruijn force-pushed the restify-request-hook branch from 804f6e2 to eda13b1 Compare November 30, 2022 09:02
@tombruijn
Copy link
Contributor Author

tombruijn commented Nov 30, 2022

Fixed the types comment and fixed some linter issues related to the code added in this PR.

Edit: I didn't mean to remove the other reviewer, but that's what happened when I asked for a re-review.

@tombruijn tombruijn requested review from blumamir and removed request for rauno56 November 30, 2022 09:04
@blumamir
Copy link
Member

Fixed the types comment and fixed some linter issues related to the code added in this PR.

Edit: I didn't mean to remove the other reviewer, but that's what happened when I asked for a re-review.

Thanks, it still shows the wrong type in the comment Restify instead of Request

@github-actions github-actions bot requested a review from rauno56 November 30, 2022 09:08
Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM

@tombruijn
Copy link
Contributor Author

tombruijn commented Nov 30, 2022

I think I fixed the compile issue that broke the build previously. The test file was missing an import that wasn't a problem when I ran the test itself.

Edit: Locally the compile task still fails for me, but because of some error from the instrumentation-koa and/or auto-instrumentations-node packages. That doesn't seem related to this PR.

@unflxw
Copy link
Contributor

unflxw commented Nov 30, 2022

@tombruijn As discussed, it would be good if this request hook, like the Express and Koa request hooks, had a layerType property, so that the hook function can know whether it's being called for a middleware or for the request handler.

@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #1312 (b537dd1) into main (97305e1) will decrease coverage by 0.40%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1312      +/-   ##
==========================================
- Coverage   96.08%   95.68%   -0.41%     
==========================================
  Files          14       20       +6     
  Lines         893     1088     +195     
  Branches      191      224      +33     
==========================================
+ Hits          858     1041     +183     
- Misses         35       47      +12     
Impacted Files Coverage Δ
...try-instrumentation-restify/src/instrumentation.ts 90.51% <100.00%> (ø)
...opentelemetry-instrumentation-restify/src/types.ts 100.00% <100.00%> (ø)
...telemetry-instrumentation-restify/src/constants.ts 100.00% <0.00%> (ø)
...opentelemetry-instrumentation-restify/src/utils.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 98.21% <0.00%> (ø)
...nstrumentation-restify/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

Add the layerType argument to the requestHook function. This is like the
following PR but for restify:
open-telemetry#1226

Move the LayerType from internal-types to types, because it's now used
in a function used by users of the instrumentation package.
@tombruijn
Copy link
Contributor Author

Like @unflxw suggested, I've added the same changes to this PR, as made for Koa in this other PR: #1226
That should really be the last change for this PR :)

@tombruijn
Copy link
Contributor Author

Hi @blumamir and/or @rauno56, is this PR ready to be merged? Let me know if I can do anything else.

@blumamir
Copy link
Member

Hi @blumamir and/or @rauno56, is this PR ready to be merged? Let me know if I can do anything else.

Yes! Thanks for the reminder 🙏
I rebased it to main and will merge when CI finish

@blumamir blumamir merged commit 4098e6a into open-telemetry:main Dec 13, 2022
@dyladan dyladan mentioned this pull request Dec 13, 2022
@tombruijn
Copy link
Contributor Author

Thank you!

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.

5 participants