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: add express plugin #666 #685

Open
wants to merge 6 commits into
base: master
from

Conversation

@vmarchaud
Copy link
Member

vmarchaud commented Jan 11, 2020

TODO:

  • add http.route in the http server span
  • add options to disable span for specific middlewares/router
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 11, 2020

Codecov Report

Merging #685 into master will increase coverage by 1.79%.
The diff coverage is 98.7%.

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   90.92%   92.71%   +1.79%     
==========================================
  Files         220      227       +7     
  Lines       10302    10446     +144     
  Branches      946      959      +13     
==========================================
+ Hits         9367     9685     +318     
+ Misses        935      761     -174
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 97.72% <ø> (ø) ⬆️
...es/opentelemetry-plugin-express/test/utils.test.ts 100% <100%> (ø)
...ckages/opentelemetry-plugin-express/src/version.ts 100% <100%> (ø)
packages/opentelemetry-plugin-http/src/utils.ts 98.02% <100%> (+0.1%) ⬆️
...lemetry-plugin-http/test/functionals/utils.test.ts 99.38% <100%> (+0.03%) ⬆️
packages/opentelemetry-plugin-express/src/types.ts 100% <100%> (ø)
packages/opentelemetry-plugin-express/src/utils.ts 95.45% <95.45%> (ø)
...ckages/opentelemetry-plugin-express/src/express.ts 98.85% <98.85%> (ø)
.../opentelemetry-plugin-express/test/express.test.ts 99.14% <99.14%> (ø)
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
... and 58 more
@vmarchaud vmarchaud added the size/XL label Jan 11, 2020
Copy link
Member

OlivierAlbertini left a comment

First pass, added few comments. Good job!

packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-http/src/utils.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
Copy link
Member

obecny left a comment

I added few comments, and have some questions too.
Last thing: could you please add some screenshots how the spans look like (for example from zipkin or something relevant etc.) ?

packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
packages/opentelemetry-plugin-express/src/express.ts Outdated Show resolved Hide resolved
vmarchaud added 4 commits Jan 11, 2020
@vmarchaud vmarchaud force-pushed the vmarchaud:add-express-plugin branch from fc25618 to 95dfd78 Jan 19, 2020
@vmarchaud vmarchaud force-pushed the vmarchaud:add-express-plugin branch from b020813 to 61088a7 Jan 19, 2020
@vmarchaud vmarchaud requested review from OlivierAlbertini and obecny Jan 19, 2020
Copy link
Contributor

dyladan left a comment

Overall this looks ok, but I am very wary of the use of a mutilated request object. In my mind this not only has the chance to break a user's application (in the unlikely event they also try to set/modify a property with the same name), but makes the express plugin an implicit dependency of the http plugin. If someone unaware of this behavior modifies this plugin in the future and changes this property, it has the chance of breaking a seemingly unrelated non-dependent plugin.

import { Request } from 'express';
import { PluginConfig, Attributes } from '@opentelemetry/types';

export const _MIDDLEWARES_STORE_PROPERTY = '__ot_middlewares';

This comment has been minimized.

Copy link
@dyladan

dyladan Jan 20, 2020

Contributor

This is a very undescriptive name with no documentation

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Jan 21, 2020

Author Member

I will update it to explain the goal of that and its link to the http plugin.

});
}
if (value === undefined) return;
(request[_MIDDLEWARES_STORE_PROPERTY] as string[]).push(value);

This comment has been minimized.

Copy link
@dyladan

dyladan Jan 20, 2020

Contributor

I am very hesitant to endorse mutilating the request object. Is there no other way to achieve this behavior?

One way I can think of to store this array is to use a WeakMap of Request to string[] within the plugin.

It looks like you're using the request object to pass the span to the http plugin, which then adds the path on the span. Without mutilating the request object I can't think of an immediately obvious way to do this, but I am very hesitant to endorse something like this which could affect user applications. My first thought is to just put the attribute on the express span which is the child of the http span and leave the http span alone. My second thought is to just use setAttribute on the parent span.

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Jan 21, 2020

Author Member

A Weakmap would not be useful since string are given as copy not reference inside the array so they will be garbage collected when the request is.

We cannot either add it into the express plugin or using setAttribute because no layer is aware of if it's the last or not. The only way is to patch the response to know when it end, and then compute the current stack of layer that have been called.
I don't think patching two times response (one for http plugin and one for express) would be good way too. Note that generally most of APM vendors (ex: datadog, newrelic) use the same technique to compute the http route.

if (layer.name === 'router') {
return {
attributes: {
[AttributeNames.EXPRESS_NAME]: layerPath,

This comment has been minimized.

Copy link
@dyladan

dyladan Jan 20, 2020

Contributor

What happens if layerPath is undefined?

This comment has been minimized.

Copy link
@vmarchaud

vmarchaud Jan 21, 2020

Author Member

It cannot be undefined here because a router without a path is just a middleware.

EDIT: so if it's a middleware, the code will not take this path anyway

@dyladan dyladan added enhancement and removed feature-request labels Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.