From 7338fcadf912c656bcbcf422a17c56f8e47b1795 Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Thu, 31 Dec 2020 17:55:06 +0800 Subject: [PATCH 1/9] Fix bug that some requests of express are not close --- src/core/PluginInstaller.ts | 4 ++-- src/core/SwPlugin.ts | 4 +++- src/plugins/AxiosPlugin.ts | 19 ++++++++++--------- src/plugins/ExpressPlugin.ts | 10 ++++++---- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/core/PluginInstaller.ts b/src/core/PluginInstaller.ts index e959501..820b2bc 100644 --- a/src/core/PluginInstaller.ts +++ b/src/core/PluginInstaller.ts @@ -32,7 +32,7 @@ while (topModule.parent) { export default class PluginInstaller { private readonly pluginDir: string; - private readonly require: (name: string) => any = topModule.require.bind(topModule); + readonly require: (name: string) => any = topModule.require.bind(topModule); private readonly resolve = (request: string) => (module.constructor as any)._resolveFilename(request, topModule); constructor() { @@ -90,7 +90,7 @@ export default class PluginInstaller { logger.info(`Installing plugin ${plugin.module} ${plugin.versions}`); - plugin.install(); + plugin.install(this); } catch (e) { if (plugin) { logger.error(`Error installing plugin ${plugin.module} ${plugin.versions}`); diff --git a/src/core/SwPlugin.ts b/src/core/SwPlugin.ts index 86b81a2..5dbbfe7 100644 --- a/src/core/SwPlugin.ts +++ b/src/core/SwPlugin.ts @@ -17,9 +17,11 @@ * */ +import PluginInstaller from './PluginInstaller'; + export default interface SwPlugin { readonly module: string; readonly versions: string; - install(): void; + install(installer: PluginInstaller): void; } diff --git a/src/plugins/AxiosPlugin.ts b/src/plugins/AxiosPlugin.ts index 1ce7191..6289be4 100644 --- a/src/plugins/AxiosPlugin.ts +++ b/src/plugins/AxiosPlugin.ts @@ -25,22 +25,23 @@ import Span from '../trace/span/Span'; import Tag from '../Tag'; import { SpanLayer } from '../proto/language-agent/Tracing_pb'; import { createLogger } from '../logging'; +import PluginInstaller from '../core/PluginInstaller'; const logger = createLogger(__filename); class AxiosPlugin implements SwPlugin { readonly module = 'axios'; readonly versions = '*'; - axios = require('axios').default; - install(): void { + install(installer: PluginInstaller): void { if (logger.isDebugEnabled()) { logger.debug('installing axios plugin'); } - this.interceptClientRequest(); + const axios = installer.require('axios').default; + this.interceptClientRequest(axios); } - private interceptClientRequest() { + private interceptClientRequest(axios: any) { const copyStatusAndStop = (span: Span, response: any) => { if (response) { if (response.status) { @@ -54,7 +55,7 @@ class AxiosPlugin implements SwPlugin { span.stop(); }; - this.axios.interceptors.request.use( + axios.interceptors.request.use( (config: any) => { config.span.resync(); @@ -73,7 +74,7 @@ class AxiosPlugin implements SwPlugin { }, ); - this.axios.interceptors.response.use( + axios.interceptors.response.use( (response: any) => { copyStatusAndStop(response.config.span, response); @@ -89,14 +90,14 @@ class AxiosPlugin implements SwPlugin { }, ); - const _request = this.axios.Axios.prototype.request; + const _request = axios.Axios.prototype.request; - this.axios.Axios.prototype.request = function(config: any) { + axios.Axios.prototype.request = function(config: any) { const { host, pathname: operation } = new URL(config.url); // TODO: this may throw invalid URL const span = ContextManager.current.newExitSpan(operation, host).start(); try { - span.component = Component.AXIOS; // TODO: add Component.AXIOS (to main Skywalking project) + span.component = Component.AXIOS; span.layer = SpanLayer.HTTP; span.peer = host; span.tag(Tag.httpURL(host + operation)); diff --git a/src/plugins/ExpressPlugin.ts b/src/plugins/ExpressPlugin.ts index 2ca1e8a..15bde6e 100644 --- a/src/plugins/ExpressPlugin.ts +++ b/src/plugins/ExpressPlugin.ts @@ -25,17 +25,18 @@ import Tag from '../Tag'; import { SpanLayer } from '../proto/language-agent/Tracing_pb'; import { ContextCarrier } from '../trace/context/ContextCarrier'; import onFinished from 'on-finished'; +import PluginInstaller from '../core/PluginInstaller'; class ExpressPlugin implements SwPlugin { readonly module = 'express'; readonly versions = '*'; - install(): void { - this.interceptServerRequest(); + install(installer: PluginInstaller): void { + this.interceptServerRequest(installer); } - private interceptServerRequest() { - const router = require('express/lib/router'); + private interceptServerRequest(installer: PluginInstaller) { + const router = installer.require('express/lib/router'); const _handle = router.handle; router.handle = function(req: IncomingMessage, res: ServerResponse, out: any) { @@ -81,6 +82,7 @@ class ExpressPlugin implements SwPlugin { } out.call(this, err); stopped -= 1; // skip first stop attempt, make sure stop executes once status code and message is set + onFinished(res, stopIfNotStopped); // this must run after any handlers deferred in 'out' }); onFinished(res, stopIfNotStopped); // this must run after any handlers deferred in 'out' From 122f94fb6d17d3688e7cb0a9fd7e6ebba7a9fdd8 Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Thu, 31 Dec 2020 20:38:06 +0800 Subject: [PATCH 2/9] Revert on-finished dependency and related codes --- dist/LICENSE | 1 - package-lock.json | 17 +++----- package.json | 2 - src/plugins/ExpressPlugin.ts | 2 +- src/plugins/HttpPlugin.ts | 83 +++++++++++++++--------------------- 5 files changed, 41 insertions(+), 64 deletions(-) diff --git a/dist/LICENSE b/dist/LICENSE index c103232..98172c1 100755 --- a/dist/LICENSE +++ b/dist/LICENSE @@ -242,7 +242,6 @@ MIT licenses The following components are provided under the MIT License. See project link for details. The text of each license is also included at licenses/LICENSE-[project].txt. - on-finished 2.3.0: https://github.com/jshttp/on-finished, MIT uuid 8.1.0: https://github.com/uuidjs/uuid, MIT winston 3.2.1: https://github.com/winstonjs/winston, MIT diff --git a/package-lock.json b/package-lock.json index 451b25a..0a8c28b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { - "name": "skywalking", - "version": "0.1.0", + "name": "skywalking-backend-js", + "version": "0.2.0", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -1188,15 +1188,6 @@ "integrity": "sha1-5IbQ2XOW15vu3QpuM/RTT/a0lz4=", "dev": true }, - "@types/on-finished": { - "version": "2.3.1", - "resolved": "https://registry.npmjs.org/@types/on-finished/-/on-finished-2.3.1.tgz", - "integrity": "sha512-mzVYaYcFs5Jd2n/O6uYIRUsFRR1cHyZLRvkLCU0E7+G5WhY0qBDAR5fUCeZbvecYOSh9ikhlesyi2UfI8B9ckQ==", - "dev": true, - "requires": { - "@types/node": "*" - } - }, "@types/prettier": { "version": "2.1.5", "resolved": "https://registry.npm.taobao.org/@types/prettier/download/@types/prettier-2.1.5.tgz", @@ -2763,7 +2754,8 @@ "ee-first": { "version": "1.1.1", "resolved": "https://registry.npm.taobao.org/ee-first/download/ee-first-1.1.1.tgz", - "integrity": "sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=" + "integrity": "sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=", + "dev": true }, "emittery": { "version": "0.7.2", @@ -6364,6 +6356,7 @@ "version": "2.3.0", "resolved": "https://registry.npmjs.org/on-finished/-/on-finished-2.3.0.tgz", "integrity": "sha1-IPEzZIGwg811M3mSoWlxqi2QaUc=", + "dev": true, "requires": { "ee-first": "1.1.1" } diff --git a/package.json b/package.json index c56cc22..e23d47e 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,6 @@ "@types/google-protobuf": "^3.7.2", "@types/jest": "^26.0.15", "@types/node": "^14.0.11", - "@types/on-finished": "^2.3.1", "@types/semver": "^7.2.0", "@types/uuid": "^8.0.0", "axios": "^0.21.0", @@ -61,7 +60,6 @@ "dependencies": { "google-protobuf": "^3.14.0", "grpc": "^1.10.1", - "on-finished": "^2.3.0", "semver": "^7.3.2", "tslib": "^2.0.3", "uuid": "^8.1.0", diff --git a/src/plugins/ExpressPlugin.ts b/src/plugins/ExpressPlugin.ts index 15bde6e..9d9a3e8 100644 --- a/src/plugins/ExpressPlugin.ts +++ b/src/plugins/ExpressPlugin.ts @@ -24,7 +24,6 @@ import { Component } from '../trace/Component'; import Tag from '../Tag'; import { SpanLayer } from '../proto/language-agent/Tracing_pb'; import { ContextCarrier } from '../trace/context/ContextCarrier'; -import onFinished from 'on-finished'; import PluginInstaller from '../core/PluginInstaller'; class ExpressPlugin implements SwPlugin { @@ -37,6 +36,7 @@ class ExpressPlugin implements SwPlugin { private interceptServerRequest(installer: PluginInstaller) { const router = installer.require('express/lib/router'); + const onFinished = installer.require('on-finished'); const _handle = router.handle; router.handle = function(req: IncomingMessage, res: ServerResponse, out: any) { diff --git a/src/plugins/HttpPlugin.ts b/src/plugins/HttpPlugin.ts index 5c2d8b2..17b52ba 100644 --- a/src/plugins/HttpPlugin.ts +++ b/src/plugins/HttpPlugin.ts @@ -26,7 +26,6 @@ import Tag from '../Tag'; import ExitSpan from '../trace/span/ExitSpan'; import { SpanLayer } from '../proto/language-agent/Tracing_pb'; import { ContextCarrier } from '../trace/context/ContextCarrier'; -import onFinished from 'on-finished'; class HttpPlugin implements SwPlugin { readonly module = 'http'; @@ -45,7 +44,7 @@ class HttpPlugin implements SwPlugin { private interceptClientRequest(module: any) { const _request = module.request; - module.request = function() { + module.request = function () { const url: URL | string | RequestOptions = arguments[0]; const { host, pathname } = @@ -59,21 +58,11 @@ class HttpPlugin implements SwPlugin { }; const operation = pathname.replace(/\?.*$/g, ''); - const span: ExitSpan = ContextManager.current.newExitSpan(operation, host).start() as ExitSpan; - let stopped = 0; // compensating if request aborted right after creation 'close' is not emitted - const stopIfNotStopped = (err?: Error | null) => { - if (stopped++) { - return; - } - span.stop(); - if (err) { - span.error(err); - } - }; + const stopIfNotStopped = () => !stopped++ ? span.stop() : null; // make sure we stop only once + const span: ExitSpan = ContextManager.current.newExitSpan(operation, host).start() as ExitSpan; try { - // TODO: these should go into span class if (span.depth === 1) { // only set HTTP if this span is not overridden by a higher level one span.component = Component.HTTP; span.layer = SpanLayer.HTTP; @@ -86,13 +75,17 @@ class HttpPlugin implements SwPlugin { span.tag(Tag.httpURL(httpURL)); } - const req: ClientRequest = _request.apply(this, arguments); + const request: ClientRequest = _request.apply(this, arguments); span.inject().items.forEach((item) => { - req.setHeader(item.key, item.value); + request.setHeader(item.key, item.value); }); - req.prependListener('response', (res) => { + request.on('close', stopIfNotStopped); + request.on('abort', () => (span.errored = true, stopIfNotStopped())); + request.on('error', (err) => (span.error(err), stopIfNotStopped())); + + request.prependListener('response', (res) => { span.resync(); span.tag(Tag.httpStatusCode(res.statusCode)); if (res.statusCode && res.statusCode >= 400) { @@ -101,15 +94,19 @@ class HttpPlugin implements SwPlugin { if (res.statusMessage) { span.tag(Tag.httpStatusMsg(res.statusMessage)); } + stopIfNotStopped(); }); - onFinished(req, stopIfNotStopped); span.async(); - return req; + return request; } catch (e) { - stopIfNotStopped(e); + if (!stopped) { // don't want to set error if exception occurs after clean close + span.error(e); + stopIfNotStopped(); + } + throw e; } }; @@ -118,7 +115,7 @@ class HttpPlugin implements SwPlugin { private interceptServerRequest(module: any) { const _emit = module.Server.prototype.emit; - module.Server.prototype.emit = function() { + module.Server.prototype.emit = function () { if (arguments[0] !== 'request') { return _emit.apply(this, arguments); } @@ -134,37 +131,27 @@ class HttpPlugin implements SwPlugin { const carrier = ContextCarrier.from(headersMap); const operation = (req.url || '/').replace(/\?.*/g, ''); - const span = ContextManager.current.newEntrySpan(operation, carrier).start(); + const span = ContextManager.current.newEntrySpan(operation, carrier); - span.component = Component.HTTP_SERVER; - span.layer = SpanLayer.HTTP; - span.peer = req.headers.host || ''; - span.tag(Tag.httpURL(span.peer + req.url)); + return ContextManager.withSpan(span, (self, args) => { + span.component = Component.HTTP_SERVER; + span.layer = SpanLayer.HTTP; + span.peer = req.headers.host || ''; + span.tag(Tag.httpURL(span.peer + req.url)); - let stopped = 0; - const stopIfNotStopped = (err: Error | null) => { - if (!stopped++) { - span.stop(); - span.tag(Tag.httpStatusCode(res.statusCode)); - if (res.statusCode && res.statusCode >= 400) { - span.errored = true; - } - if (err) { - span.error(err); - } - if (res.statusMessage) { - span.tag(Tag.httpStatusMsg(res.statusMessage)); - } + const ret = _emit.apply(self, args); + + span.tag(Tag.httpStatusCode(res.statusCode)); + if (res.statusCode && res.statusCode >= 400) { + span.errored = true; + } + if (res.statusMessage) { + span.tag(Tag.httpStatusMsg(res.statusMessage)); } - }; - onFinished(res, stopIfNotStopped); - try { - return _emit.apply(this, arguments); - } catch (e) { - stopIfNotStopped(e); - throw e; - } + return ret; + + }, this, arguments); }; } } From b91884037d06e02cc2e775348322d458f4c470c5 Mon Sep 17 00:00:00 2001 From: Zhenxu Ke Date: Thu, 31 Dec 2020 21:58:58 +0800 Subject: [PATCH 3/9] Update HttpPlugin.ts --- src/plugins/HttpPlugin.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/plugins/HttpPlugin.ts b/src/plugins/HttpPlugin.ts index 17b52ba..e679ec9 100644 --- a/src/plugins/HttpPlugin.ts +++ b/src/plugins/HttpPlugin.ts @@ -94,7 +94,6 @@ class HttpPlugin implements SwPlugin { if (res.statusMessage) { span.tag(Tag.httpStatusMsg(res.statusMessage)); } - stopIfNotStopped(); }); span.async(); From f1c0228045a8ff7bdd20657a6b18b0d613322b2c Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Thu, 31 Dec 2020 22:41:00 +0800 Subject: [PATCH 4/9] Try `end` -> `close` --- tests/plugins/express/client.ts | 2 +- tests/plugins/express/server.ts | 2 +- tests/plugins/http/client.ts | 2 +- tests/plugins/http/server.ts | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/plugins/express/client.ts b/tests/plugins/express/client.ts index 20e0487..2e4efa6 100644 --- a/tests/plugins/express/client.ts +++ b/tests/plugins/express/client.ts @@ -33,7 +33,7 @@ app.get('/express', (req, res) => { .request(`http://${process.env.SERVER || 'localhost:5000'}${req.url}`, (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('end', () => res.send(data)); + r.on('close', () => res.send(data)); }) .end(); }); diff --git a/tests/plugins/express/server.ts b/tests/plugins/express/server.ts index 9ce3346..8bb15ef 100644 --- a/tests/plugins/express/server.ts +++ b/tests/plugins/express/server.ts @@ -33,7 +33,7 @@ app.get('/express', (req, res) => { .request('http://httpbin.org/json', (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('end', () => res.send(data)); + r.on('close', () => res.send(data)); }) .end(); }); diff --git a/tests/plugins/http/client.ts b/tests/plugins/http/client.ts index c1ee8a9..cbdc2c6 100644 --- a/tests/plugins/http/client.ts +++ b/tests/plugins/http/client.ts @@ -30,7 +30,7 @@ const server = http.createServer((req, res) => { .request(`http://${process.env.SERVER || 'localhost:5000'}${req.url}`, (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('end', () => res.end(data)); + r.on('close', () => res.end(data)); }) .end(); }); diff --git a/tests/plugins/http/server.ts b/tests/plugins/http/server.ts index bf362c7..87eada3 100644 --- a/tests/plugins/http/server.ts +++ b/tests/plugins/http/server.ts @@ -30,7 +30,7 @@ const server = http.createServer((req, res) => { .request('http://httpbin.org/json', (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('end', () => res.end(data)); + r.on('close', () => res.end(data)); }) .end(); }); From e29e9c476d2883e619ba01d6d0d7ea0a94648128 Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Thu, 31 Dec 2020 23:06:09 +0800 Subject: [PATCH 5/9] Fix expected data --- tests/plugins/axios/expected.data.yaml | 52 ++++++++++++------------ tests/plugins/express/expected.data.yaml | 52 ++++++++++++------------ tests/plugins/http/expected.data.yaml | 52 ++++++++++++------------ 3 files changed, 78 insertions(+), 78 deletions(-) diff --git a/tests/plugins/axios/expected.data.yaml b/tests/plugins/axios/expected.data.yaml index 1f24544..0fb7385 100644 --- a/tests/plugins/axios/expected.data.yaml +++ b/tests/plugins/axios/expected.data.yaml @@ -21,24 +21,6 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: /json - operationId: 0 - parentSpanId: 0 - spanId: 1 - spanLayer: Http - tags: - - key: http.url - value: httpbin.org:80/json - - key: http.status.code - value: '200' - - key: http.status.msg - value: OK - startTime: gt 0 - endTime: gt 0 - componentId: 4005 - spanType: Exit - peer: httpbin.org - skipAnalysis: false - operationName: /axios operationId: 0 parentSpanId: -1 @@ -66,19 +48,14 @@ segmentItems: spanType: Entry peer: server:5000 skipAnalysis: false - - serviceName: client - segmentSize: 1 - segments: - - segmentId: not null - spans: - - operationName: /axios + - operationName: /json operationId: 0 parentSpanId: 0 spanId: 1 spanLayer: Http tags: - key: http.url - value: server:5000/axios + value: httpbin.org:80/json - key: http.status.code value: '200' - key: http.status.msg @@ -87,8 +64,13 @@ segmentItems: endTime: gt 0 componentId: 4005 spanType: Exit - peer: server:5000 + peer: httpbin.org skipAnalysis: false + - serviceName: client + segmentSize: 1 + segments: + - segmentId: not null + spans: - operationName: /axios operationId: 0 parentSpanId: -1 @@ -107,3 +89,21 @@ segmentItems: spanType: Entry peer: localhost:5001 skipAnalysis: false + - operationName: /axios + operationId: 0 + parentSpanId: 0 + spanId: 1 + spanLayer: Http + tags: + - key: http.url + value: server:5000/axios + - key: http.status.code + value: '200' + - key: http.status.msg + value: OK + startTime: gt 0 + endTime: gt 0 + componentId: 4005 + spanType: Exit + peer: server:5000 + skipAnalysis: false diff --git a/tests/plugins/express/expected.data.yaml b/tests/plugins/express/expected.data.yaml index 4f6311a..9cf0fad 100644 --- a/tests/plugins/express/expected.data.yaml +++ b/tests/plugins/express/expected.data.yaml @@ -21,24 +21,6 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: /json - operationId: 0 - parentSpanId: 0 - spanId: 1 - spanLayer: Http - tags: - - key: http.url - value: httpbin.org/json - - key: http.status.code - value: '200' - - key: http.status.msg - value: OK - startTime: gt 0 - endTime: gt 0 - componentId: 2 - spanType: Exit - peer: httpbin.org - skipAnalysis: false - operationName: /express operationId: 0 parentSpanId: -1 @@ -66,19 +48,14 @@ segmentItems: spanType: Entry peer: server:5000 skipAnalysis: false - - serviceName: client - segmentSize: 1 - segments: - - segmentId: not null - spans: - - operationName: /express + - operationName: /json operationId: 0 parentSpanId: 0 spanId: 1 spanLayer: Http tags: - key: http.url - value: server:5000/express + value: httpbin.org/json - key: http.status.code value: '200' - key: http.status.msg @@ -87,8 +64,13 @@ segmentItems: endTime: gt 0 componentId: 2 spanType: Exit - peer: server:5000 + peer: httpbin.org skipAnalysis: false + - serviceName: client + segmentSize: 1 + segments: + - segmentId: not null + spans: - operationName: /express operationId: 0 parentSpanId: -1 @@ -107,3 +89,21 @@ segmentItems: spanType: Entry peer: localhost:5001 skipAnalysis: false + - operationName: /express + operationId: 0 + parentSpanId: 0 + spanId: 1 + spanLayer: Http + tags: + - key: http.url + value: server:5000/express + - key: http.status.code + value: '200' + - key: http.status.msg + value: OK + startTime: gt 0 + endTime: gt 0 + componentId: 2 + spanType: Exit + peer: server:5000 + skipAnalysis: false diff --git a/tests/plugins/http/expected.data.yaml b/tests/plugins/http/expected.data.yaml index 1e9e6f2..732f6eb 100644 --- a/tests/plugins/http/expected.data.yaml +++ b/tests/plugins/http/expected.data.yaml @@ -21,24 +21,6 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: /json - operationId: 0 - parentSpanId: 0 - spanId: 1 - spanLayer: Http - startTime: gt 0 - endTime: gt 0 - componentId: 2 - spanType: Exit - peer: httpbin.org - skipAnalysis: false - tags: - - key: http.url - value: httpbin.org/json - - key: http.status.code - value: '200' - - key: http.status.msg - value: OK - operationName: /test operationId: 0 parentSpanId: -1 @@ -66,12 +48,7 @@ segmentItems: parentServiceInstance: not null parentService: client traceId: not null - - serviceName: client - segmentSize: 1 - segments: - - segmentId: not null - spans: - - operationName: /test + - operationName: /json operationId: 0 parentSpanId: 0 spanId: 1 @@ -80,15 +57,20 @@ segmentItems: endTime: gt 0 componentId: 2 spanType: Exit - peer: server:5000 + peer: httpbin.org skipAnalysis: false tags: - key: http.url - value: server:5000/test + value: httpbin.org/json - key: http.status.code value: '200' - key: http.status.msg value: OK + - serviceName: client + segmentSize: 1 + segments: + - segmentId: not null + spans: - operationName: /test operationId: 0 parentSpanId: -1 @@ -107,3 +89,21 @@ segmentItems: value: '200' - key: http.status.msg value: OK + - operationName: /test + operationId: 0 + parentSpanId: 0 + spanId: 1 + spanLayer: Http + startTime: gt 0 + endTime: gt 0 + componentId: 2 + spanType: Exit + peer: server:5000 + skipAnalysis: false + tags: + - key: http.url + value: server:5000/test + - key: http.status.code + value: '200' + - key: http.status.msg + value: OK From f0a5e8ba58af7a64cbfe68094f310b71d5cbbfef Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Thu, 31 Dec 2020 23:30:25 +0800 Subject: [PATCH 6/9] Fix expected data --- tests/plugins/http/expected.data.yaml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/plugins/http/expected.data.yaml b/tests/plugins/http/expected.data.yaml index 732f6eb..dfb19ac 100644 --- a/tests/plugins/http/expected.data.yaml +++ b/tests/plugins/http/expected.data.yaml @@ -37,8 +37,6 @@ segmentItems: value: server:5000/test - key: http.status.code value: '200' - - key: http.status.msg - value: OK refs: - parentEndpoint: '' networkAddress: server:5000 @@ -87,8 +85,6 @@ segmentItems: value: localhost:5001/test - key: http.status.code value: '200' - - key: http.status.msg - value: OK - operationName: /test operationId: 0 parentSpanId: 0 From 384873b314028ae09f591733f9723ad5e59ea47a Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Fri, 1 Jan 2021 12:05:52 +0800 Subject: [PATCH 7/9] Fix test --- tests/plugins/axios/expected.data.yaml | 4 -- tests/plugins/express/client.ts | 2 +- tests/plugins/express/expected.data.yaml | 52 ++++++++++++------------ tests/plugins/express/server.ts | 2 +- tests/plugins/http/client.ts | 2 +- tests/plugins/http/server.ts | 2 +- 6 files changed, 30 insertions(+), 34 deletions(-) diff --git a/tests/plugins/axios/expected.data.yaml b/tests/plugins/axios/expected.data.yaml index 0fb7385..fb28cf4 100644 --- a/tests/plugins/axios/expected.data.yaml +++ b/tests/plugins/axios/expected.data.yaml @@ -31,8 +31,6 @@ segmentItems: value: server:5000/axios - key: http.status.code value: '200' - - key: http.status.msg - value: OK refs: - parentEndpoint: '' networkAddress: server:5000 @@ -81,8 +79,6 @@ segmentItems: value: localhost:5001/axios - key: http.status.code value: '200' - - key: http.status.msg - value: OK startTime: gt 0 endTime: gt 0 componentId: 49 diff --git a/tests/plugins/express/client.ts b/tests/plugins/express/client.ts index 2e4efa6..20e0487 100644 --- a/tests/plugins/express/client.ts +++ b/tests/plugins/express/client.ts @@ -33,7 +33,7 @@ app.get('/express', (req, res) => { .request(`http://${process.env.SERVER || 'localhost:5000'}${req.url}`, (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('close', () => res.send(data)); + r.on('end', () => res.send(data)); }) .end(); }); diff --git a/tests/plugins/express/expected.data.yaml b/tests/plugins/express/expected.data.yaml index 9cf0fad..4f6311a 100644 --- a/tests/plugins/express/expected.data.yaml +++ b/tests/plugins/express/expected.data.yaml @@ -21,6 +21,24 @@ segmentItems: segments: - segmentId: not null spans: + - operationName: /json + operationId: 0 + parentSpanId: 0 + spanId: 1 + spanLayer: Http + tags: + - key: http.url + value: httpbin.org/json + - key: http.status.code + value: '200' + - key: http.status.msg + value: OK + startTime: gt 0 + endTime: gt 0 + componentId: 2 + spanType: Exit + peer: httpbin.org + skipAnalysis: false - operationName: /express operationId: 0 parentSpanId: -1 @@ -48,14 +66,19 @@ segmentItems: spanType: Entry peer: server:5000 skipAnalysis: false - - operationName: /json + - serviceName: client + segmentSize: 1 + segments: + - segmentId: not null + spans: + - operationName: /express operationId: 0 parentSpanId: 0 spanId: 1 spanLayer: Http tags: - key: http.url - value: httpbin.org/json + value: server:5000/express - key: http.status.code value: '200' - key: http.status.msg @@ -64,13 +87,8 @@ segmentItems: endTime: gt 0 componentId: 2 spanType: Exit - peer: httpbin.org + peer: server:5000 skipAnalysis: false - - serviceName: client - segmentSize: 1 - segments: - - segmentId: not null - spans: - operationName: /express operationId: 0 parentSpanId: -1 @@ -89,21 +107,3 @@ segmentItems: spanType: Entry peer: localhost:5001 skipAnalysis: false - - operationName: /express - operationId: 0 - parentSpanId: 0 - spanId: 1 - spanLayer: Http - tags: - - key: http.url - value: server:5000/express - - key: http.status.code - value: '200' - - key: http.status.msg - value: OK - startTime: gt 0 - endTime: gt 0 - componentId: 2 - spanType: Exit - peer: server:5000 - skipAnalysis: false diff --git a/tests/plugins/express/server.ts b/tests/plugins/express/server.ts index 8bb15ef..9ce3346 100644 --- a/tests/plugins/express/server.ts +++ b/tests/plugins/express/server.ts @@ -33,7 +33,7 @@ app.get('/express', (req, res) => { .request('http://httpbin.org/json', (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('close', () => res.send(data)); + r.on('end', () => res.send(data)); }) .end(); }); diff --git a/tests/plugins/http/client.ts b/tests/plugins/http/client.ts index cbdc2c6..c1ee8a9 100644 --- a/tests/plugins/http/client.ts +++ b/tests/plugins/http/client.ts @@ -30,7 +30,7 @@ const server = http.createServer((req, res) => { .request(`http://${process.env.SERVER || 'localhost:5000'}${req.url}`, (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('close', () => res.end(data)); + r.on('end', () => res.end(data)); }) .end(); }); diff --git a/tests/plugins/http/server.ts b/tests/plugins/http/server.ts index 87eada3..bf362c7 100644 --- a/tests/plugins/http/server.ts +++ b/tests/plugins/http/server.ts @@ -30,7 +30,7 @@ const server = http.createServer((req, res) => { .request('http://httpbin.org/json', (r) => { let data = ''; r.on('data', (chunk) => (data += chunk)); - r.on('close', () => res.end(data)); + r.on('end', () => res.end(data)); }) .end(); }); From 0f68a81b0d651a85b803826dfdc76559a0821388 Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Fri, 1 Jan 2021 14:37:15 +0800 Subject: [PATCH 8/9] Fix test --- src/plugins/AxiosPlugin.ts | 3 -- src/trace/context/SpanContext.ts | 60 ++++++++++++------------ src/trace/span/Span.ts | 5 -- tests/plugins/express/expected.data.yaml | 52 ++++++++++---------- 4 files changed, 57 insertions(+), 63 deletions(-) diff --git a/src/plugins/AxiosPlugin.ts b/src/plugins/AxiosPlugin.ts index 6289be4..0a9df5d 100644 --- a/src/plugins/AxiosPlugin.ts +++ b/src/plugins/AxiosPlugin.ts @@ -57,8 +57,6 @@ class AxiosPlugin implements SwPlugin { axios.interceptors.request.use( (config: any) => { - config.span.resync(); - (config.span as Span).inject().items.forEach((item) => { config.headers.common[item.key] = item.value; }); @@ -101,7 +99,6 @@ class AxiosPlugin implements SwPlugin { span.layer = SpanLayer.HTTP; span.peer = host; span.tag(Tag.httpURL(host + operation)); - span.async(); return _request.call(this, { ...config, span }); } catch (e) { diff --git a/src/trace/context/SpanContext.ts b/src/trace/context/SpanContext.ts index 6eb2c59..20bf96c 100644 --- a/src/trace/context/SpanContext.ts +++ b/src/trace/context/SpanContext.ts @@ -64,13 +64,6 @@ export default class SpanContext implements Context { } newEntrySpan(operation: string, carrier?: ContextCarrier): Span { - if (logger.isDebugEnabled()) { - logger.debug('Creating entry span', { - parentId: this.parentId, - executionAsyncId: executionAsyncId(), - }); - } - let span = this.ignoreCheck(operation, SpanType.ENTRY); if (span) @@ -79,6 +72,13 @@ export default class SpanContext implements Context { const spans = ContextManager.spansDup(); const parent = spans[spans.length - 1]; + if (logger.isDebugEnabled()) { + logger.debug('Creating entry span', { + spans, + parent, + }); + } + if (parent && parent.type === SpanType.ENTRY) { span = parent; parent.operation = operation; @@ -100,13 +100,6 @@ export default class SpanContext implements Context { } newExitSpan(operation: string, peer: string): Span { - if (logger.isDebugEnabled()) { - logger.debug('Creating exit span', { - parentId: this.parentId, - executionAsyncId: executionAsyncId(), - }); - } - let span = this.ignoreCheck(operation, SpanType.EXIT); if (span) @@ -115,6 +108,15 @@ export default class SpanContext implements Context { const spans = ContextManager.spansDup(); const parent = spans[spans.length - 1]; + if (logger.isDebugEnabled()) { + logger.debug('Creating exit span', { + operation, + parent, + spans, + peer, + }); + } + if (parent && parent.type === SpanType.EXIT) { span = parent; @@ -132,13 +134,6 @@ export default class SpanContext implements Context { } newLocalSpan(operation: string): Span { - if (logger.isDebugEnabled()) { - logger.debug('Creating local span', { - parentId: this.parentId, - executionAsyncId: executionAsyncId(), - }); - } - const span = this.ignoreCheck(operation, SpanType.LOCAL); if (span) @@ -146,6 +141,13 @@ export default class SpanContext implements Context { ContextManager.spansDup(); + if (logger.isDebugEnabled()) { + logger.debug('Creating local span', { + parentId: this.parentId, + executionAsyncId: executionAsyncId(), + }); + } + return new LocalSpan({ id: this.spanId++, parentId: this.parentId, @@ -155,8 +157,8 @@ export default class SpanContext implements Context { } start(span: Span): Context { - logger.debug('Starting span', { - span: span.operation, + logger.debug(`Starting span ${span.operation}`, { + span, spans: ContextManager.spans, nSpans: this.nSpans, }); @@ -170,8 +172,8 @@ export default class SpanContext implements Context { } stop(span: Span): boolean { - logger.debug('Stopping span', { - span: span.operation, + logger.debug(`Stopping span ${span.operation}`, { + span, spans: ContextManager.spans, nSpans: this.nSpans, }); @@ -193,8 +195,8 @@ export default class SpanContext implements Context { } async(span: Span) { - logger.debug('Async span', { - span: span.operation, + logger.debug(`Async span ${span.operation}`, { + span, spans: ContextManager.spans, nSpans: this.nSpans, }); @@ -211,8 +213,8 @@ export default class SpanContext implements Context { } resync(span: Span) { - logger.debug('Resync span', { - span: span.operation, + logger.debug(`Resync span ${span.operation}`, { + span, spans: ContextManager.spans, nSpans: this.nSpans, }); diff --git a/src/trace/span/Span.ts b/src/trace/span/Span.ts index f97cb0a..873f4e1 100644 --- a/src/trace/span/Span.ts +++ b/src/trace/span/Span.ts @@ -72,32 +72,27 @@ export default abstract class Span { } start(): this { - logger.debug(`Starting span ${this.operation}`, this); this.startTime = new Date().getTime(); this.context.start(this); return this; } stop(): this { - logger.debug(`Stopping span ${this.operation}`, this); this.context.stop(this); return this; } async(): this { - logger.debug(`Async span ${this.operation}`, this); this.context.async(this); return this; } resync(): this { - logger.debug(`Resync span ${this.operation}`, this); this.context.resync(this); return this; } finish(segment: Segment): boolean { - logger.debug('Finishing span', this); this.endTime = new Date().getTime(); segment.archive(this); return true; diff --git a/tests/plugins/express/expected.data.yaml b/tests/plugins/express/expected.data.yaml index 4f6311a..9cf0fad 100644 --- a/tests/plugins/express/expected.data.yaml +++ b/tests/plugins/express/expected.data.yaml @@ -21,24 +21,6 @@ segmentItems: segments: - segmentId: not null spans: - - operationName: /json - operationId: 0 - parentSpanId: 0 - spanId: 1 - spanLayer: Http - tags: - - key: http.url - value: httpbin.org/json - - key: http.status.code - value: '200' - - key: http.status.msg - value: OK - startTime: gt 0 - endTime: gt 0 - componentId: 2 - spanType: Exit - peer: httpbin.org - skipAnalysis: false - operationName: /express operationId: 0 parentSpanId: -1 @@ -66,19 +48,14 @@ segmentItems: spanType: Entry peer: server:5000 skipAnalysis: false - - serviceName: client - segmentSize: 1 - segments: - - segmentId: not null - spans: - - operationName: /express + - operationName: /json operationId: 0 parentSpanId: 0 spanId: 1 spanLayer: Http tags: - key: http.url - value: server:5000/express + value: httpbin.org/json - key: http.status.code value: '200' - key: http.status.msg @@ -87,8 +64,13 @@ segmentItems: endTime: gt 0 componentId: 2 spanType: Exit - peer: server:5000 + peer: httpbin.org skipAnalysis: false + - serviceName: client + segmentSize: 1 + segments: + - segmentId: not null + spans: - operationName: /express operationId: 0 parentSpanId: -1 @@ -107,3 +89,21 @@ segmentItems: spanType: Entry peer: localhost:5001 skipAnalysis: false + - operationName: /express + operationId: 0 + parentSpanId: 0 + spanId: 1 + spanLayer: Http + tags: + - key: http.url + value: server:5000/express + - key: http.status.code + value: '200' + - key: http.status.msg + value: OK + startTime: gt 0 + endTime: gt 0 + componentId: 2 + spanType: Exit + peer: server:5000 + skipAnalysis: false From 698d27ae47f262bba8c18548867991a3bf2ca9bd Mon Sep 17 00:00:00 2001 From: kezhenxu94 Date: Fri, 1 Jan 2021 22:34:18 +0800 Subject: [PATCH 9/9] Comment instead of removing --- src/plugins/AxiosPlugin.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/plugins/AxiosPlugin.ts b/src/plugins/AxiosPlugin.ts index 0a9df5d..7a1ac28 100644 --- a/src/plugins/AxiosPlugin.ts +++ b/src/plugins/AxiosPlugin.ts @@ -57,6 +57,8 @@ class AxiosPlugin implements SwPlugin { axios.interceptors.request.use( (config: any) => { + // config.span.resync(); // TODO: fix this https://github.com/apache/skywalking-nodejs/pull/20#issuecomment-753323425 + (config.span as Span).inject().items.forEach((item) => { config.headers.common[item.key] = item.value; }); @@ -99,6 +101,7 @@ class AxiosPlugin implements SwPlugin { span.layer = SpanLayer.HTTP; span.peer = host; span.tag(Tag.httpURL(host + operation)); + // span.async(); TODO: fix this https://github.com/apache/skywalking-nodejs/pull/20#issuecomment-753323425 return _request.call(this, { ...config, span }); } catch (e) {