feat: restify instrumentation #416

rauno56 · 2021-04-08T13:35:53Z

Which problem is this PR solving?

This is adding restify instrumentation.

Short description of the changes

Restify has a somewhat similar API to express and is created for the same purpose - building APIs. This instrumentation wraps all handlers to generate a span for each of them.

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

codecov · 2021-04-08T13:45:37Z

Codecov Report

Merging #416 (cc1383e) into main (2dceaf3) will not change coverage.
The diff coverage is n/a.

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

@@           Coverage Diff           @@
##             main     #416   +/-   ##
=======================================
  Coverage   95.83%   95.83%           
=======================================
  Files          15       15           
  Lines         624      624           
  Branches       94       94           
=======================================
  Hits          598      598           
  Misses         26       26

owais · 2021-04-12T11:39:54Z

examples/restify/client.js

+  // the span, which is created to track work that happens outside of the
+  // request lifecycle entirely.
+  const span = tracer.startSpan(`makeRequest to ${path}`);
+  api.context.with(api.setSpan(api.context.active(), span), () => {


Do we need to include manual span creation in the example? I think we should keep the client as simple as possible for an example focusing on server/restify instrumentation. HTTP instrumentation should generate a client span anyway.

True. It would make sense if the HTTP instrumentation was not there. I'll remove that.

owais · 2021-04-12T11:43:23Z

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

+
+const { diag } = api;
+const RESTIFY_MW_METHODS = ['use', 'pre'];
+const RESTIFY_METHODS = ['del', 'get', 'head', 'opts', 'post', 'put', 'patch'];


Does restify support custom HTTP methods? If so, will this instrumentation be able to work with them?

No. Everything that allows adding a handler is listed here. There's a "wildcard"-way of reaching to the API of find-my-way which is mentioned here in restify docs, but even that has to be done through one of these functions.

However, I'm not testing for that shortcut. Will add a test for that.

Restify does not support custom HTTP methods, it turns out. It overwrites it with the one specified with function used to register the handler. Added a test to make sure the more verbose API works as expected.

seemk · 2021-04-12T12:56:09Z

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

+        if (this._isDisabled) {
+          return handler(req, res, next);
+        }
+        // const parentSpan = api.getSpan(api?.context?.active());


Need to update the name of the parent span here for incoming requests, e.g. as is in express: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-express/src/express.ts#L193-L208

seemk · 2021-04-12T13:08:09Z

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

+    return module;
+  }
+
+  private _middlewarePatcher(original: Function, methodName?: string) {


I think middleware spans should have middleware name as the span name (not sure if possibly with restify), e.g. the operation tree looks like this currently, where each middleware operation name is the route name:

However the express instrumentation contains middleware names:

Also I think middleware spans should have the incoming request span as its parent.

Yep, it's possible. Express takes the name of the function it seems, which can be undefined at times, but still better than nothing. Thanks!

Also I think middleware spans should have the incoming request span as its parent.

I generally agree but don't want to change that because there might not be a request span if HTTP instrumentation is not enabled, so the structure would be different between HTTP instrumentation enabled and not.

seemk · 2021-04-12T13:24:26Z

Seems to work fine with restify 5.2.1

johnbley

In general, LGTM. Thanks for this significant feature contribution!

johnbley · 2021-04-13T17:59:12Z

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

+      return original.call(
+        this,
+        path,
+        ...instrumentation._handlerPatcher(


I like how cleanly you've kept the reuse between methodPatcher and middlewarePatcher - not too complex but still keeping the essential bits parameterized instead of copied+pasted.

dyladan

Looks awesome thanks for contributing

vmarchaud

lgtm !

obecny

overall lgtm, just few issues

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts

examples/restify/tracer.js

plugins/node/opentelemetry-instrumentation-restify/package.json

plugins/node/opentelemetry-instrumentation-restify/README.md

rauno56 · 2021-04-14T09:55:08Z

I think I addressed everything except two totally valid points @obecny, brought up, I don't necessarily think are worth changing and would like a second opinion on:

Renaming c to constants. Unnecessarily verbose in such a use-case in my view.
Use of lodash.once. I think we should use quality well-known packages and not reimplement utilities, which would be more costly in the long run, especially in the case where some(most?) distros ship tons of instrumentations, used or not, anyways. We cannot avoid bringing in any 3rd party packages and we have done so in the past(shimmer, require-in-the-middle, etc.).

I don't have a strong technical preference in either of those points, more of a question of how much bikeshedding we want to go through in the long run and for me to calibrate how important maintainers generally think such questions are.

vmarchaud · 2021-04-14T20:16:43Z

I agree with @obecny here on both points:

i prefer verbosity for the sake of readability
- our project is open source, so we expect a lot of people will review our code having constants over c make it easy to understand what the variable is, specially when reading on github and not within an IDE)
i prefer to not use a dependency to avoid having few more lines in our code
- I don't agree with you by comparing lodash.once (which is 5 lines long that anyone could write without specific knowledge) and shimmer/require-in-the-middle which are battle tested with complex problem within NodeJS
- Specially in this case i don't think its actually needed, the next callback shouldn't be called twice right ? And even if it did, nothing will happens (it will generate a warning log on the diagnostic logger which isn't logged by default) so its safe to remove it altogether

I don't have a strong technical preference in either of those points, more of a question of how much bikeshedding we want to go through in the long run and for me to calibrate how important maintainers generally think such questions are.

I agree with you on this however: that there are both technical preference. I don't like blocking PRs for those kind of choices (so i'll not) but i strongly recommend addressing them.

rauno56 · 2021-04-15T07:22:52Z

Specially in this case i don't think its actually needed, the next callback shouldn't be called twice right ?

Yes, it is not supposed to, but restify doesn't error if it is called twice, just ignores it. There are other cases where it's not called at all. That did make me think about the cases with promises. Added checks for those.

Removed lodash.once and renamed c.

Thanks a lot @vmarchaud and @obecny for your input. It's highly appreciated! 🙏

rauno56 · 2021-04-15T07:26:11Z

I don't agree with you by comparing lodash.once (which is 5 lines long that anyone could write without specific knowledge) and shimmer/require-in-the-middle which are battle tested with complex problem within NodeJS

I do wonder whether we should come up with another solution than implementing stuff ourselves if we have a utility in 3 instrumentations? 5? 15?

vmarchaud · 2021-04-15T07:36:13Z

I do wonder whether we should come up with another solution than implementing stuff ourselves if we have a utility in 3 instrumentations? 5? 15?

We generally have them on the core package (https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-core/src/utils) or the instrumentation package itself (https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-instrumentation/src/utils.ts)

vmarchaud

thanks taking the time to addressing all feedback :)

obecny

lgtm, thx for changes

obecny · 2021-04-15T19:41:20Z

examples/restify/package.json

+{
+  "name": "restify-example",
+  "private": true,
+  "version": "0.18.0",


Nit: 0.15.0 in core we have 0.18.x

Version in examples are not kept in sync in this repo, i think it would be better to do it with lerna instead

EDit: #428

rauno56 commented Apr 8, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-restify/src/instrumentation.ts Outdated Show resolved Hide resolved

rauno56 force-pushed the feat/restify-instrumentation branch 2 times, most recently from 6718f96 to 44607ca Compare April 12, 2021 09:54

owais reviewed Apr 12, 2021

View reviewed changes

seemk reviewed Apr 12, 2021

View reviewed changes

rauno56 marked this pull request as ready for review April 13, 2021 10:44

rauno56 requested a review from a team April 13, 2021 10:44

rauno56 changed the title ~~help-needed: restify instrumentation~~ feat: restify instrumentation Apr 13, 2021

johnbley approved these changes Apr 13, 2021

View reviewed changes

dyladan approved these changes Apr 13, 2021

View reviewed changes

vmarchaud approved these changes Apr 13, 2021

View reviewed changes

obecny reviewed Apr 13, 2021

View reviewed changes

vmarchaud self-requested a review April 14, 2021 20:11

vmarchaud approved these changes Apr 15, 2021

View reviewed changes

vmarchaud requested a review from obecny April 15, 2021 07:38

rauno56 added 8 commits April 15, 2021 18:52

feat: initial implementation of restify instrumentation

0149bcf

feat: create spans instead of altering parent spans

96f73e6

feat: clean examples up

4f920f5

feat: make exporter configurable like other examples

92af233

feat: implement disabling restify instrumentation

3c37d7f

test: be more verbose in tests and check all spans

32b8931

fix: remove the handlers from req, those events seem not to be fired

ee64e4d

refactor: export the instrumentation as default

b6c8318

rauno56 added 21 commits April 15, 2021 18:52

fix: thrown errors are generally not caught in restify

a46adb7

feat: enable the instrumentation for all versions after 4.0.0

5d67867

docs: update docs and metadata in package.json and README.md files

c4657df

style: fix linting errors

cbbdea0

style: make _moduleVersion optional

c760527

feat: use more descriptive span name

4ca8036

feat: rename HTTP span

e9ab22e

feat: remove creating additional span on the client side

0dc22aa

docs: remove TODO

6c380a2

feat: update version

3b8072b

test: test more verbose API

fa55dab

fix: update version

ac8d41b

fix: update the example in README

520d54e

fix: reference @opentelemetry/instrumentation-http correctly

b5be54c

feat: remove lodash.once

14772ba

style: rename constants to be more verbose

c8914d1

feat: handle cases with promises

88f6052

feat: support async and promise-returning handlers

df87efd

style: use RestifyInstrumentation as the import name

4b58434

feat: let the example be broken until the publish for consistency

c4dee5c

fix: remove lodash.once types

f8bcb5c

rauno56 force-pushed the feat/restify-instrumentation branch from 67184f2 to f8bcb5c Compare April 15, 2021 15:53

Merge branch 'main' into feat/restify-instrumentation

fd906b1

obecny approved these changes Apr 15, 2021

View reviewed changes

obecny added the enhancement New feature or request label Apr 15, 2021

Merge branch 'main' into feat/restify-instrumentation

f4f4b08

vmarchaud merged commit b7d84d1 into open-telemetry:main Apr 16, 2021

rauno56 mentioned this pull request Apr 16, 2021

Add plugin for restify #134

Closed

rauno56 mentioned this pull request May 12, 2021

feat: router instrumentation #458

Merged

rauno56 deleted the feat/restify-instrumentation branch May 13, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: restify instrumentation #416

feat: restify instrumentation #416

rauno56 commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

owais Apr 12, 2021

rauno56 Apr 13, 2021

owais Apr 12, 2021

rauno56 Apr 12, 2021

rauno56 Apr 13, 2021

seemk Apr 12, 2021

seemk Apr 12, 2021

rauno56 Apr 13, 2021

seemk commented Apr 12, 2021 •

edited

Loading

johnbley left a comment

johnbley Apr 13, 2021

rauno56 Apr 14, 2021

dyladan left a comment

vmarchaud left a comment

obecny left a comment

rauno56 commented Apr 14, 2021

vmarchaud commented Apr 14, 2021 •

edited

Loading

rauno56 commented Apr 15, 2021

rauno56 commented Apr 15, 2021

vmarchaud commented Apr 15, 2021

vmarchaud left a comment

obecny left a comment

obecny Apr 15, 2021

vmarchaud Apr 16, 2021 •

edited

Loading

feat: restify instrumentation #416

feat: restify instrumentation #416

Conversation

rauno56 commented Apr 8, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seemk commented Apr 12, 2021 • edited Loading

johnbley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

vmarchaud left a comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

rauno56 commented Apr 14, 2021

vmarchaud commented Apr 14, 2021 • edited Loading

rauno56 commented Apr 15, 2021

rauno56 commented Apr 15, 2021

vmarchaud commented Apr 15, 2021

vmarchaud left a comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

rauno56 commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

seemk commented Apr 12, 2021 •

edited

Loading

vmarchaud commented Apr 14, 2021 •

edited

Loading

vmarchaud Apr 16, 2021 •

edited

Loading