fix: AxiosPlugin async() / resync() (#21)
Co-authored-by: kezhenxu94 <kezhenxu94@apache.org>
diff --git a/src/config/AgentConfig.ts b/src/config/AgentConfig.ts
index 2eaa156..8c9031b 100644
--- a/src/config/AgentConfig.ts
+++ b/src/config/AgentConfig.ts
@@ -38,9 +38,9 @@
const ignorePath = '^(?:' + config.traceIgnorePath!.split(',').map(
(s1) => s1.trim().split('**').map(
(s2) => s2.split('*').map(
- (s3) => s3.split('?').map(escapeRegExp).join('[^/]') // replaces "**"
+ (s3) => s3.split('?').map(escapeRegExp).join('[^/]') // replaces "?"
).join('[^/]*') // replaces "*"
- ).join('(?:(?:[^/]+\.)*[^/]+)?') // replaces "?"
+ ).join('(?:(?:[^/]+\.)*[^/]+)?') // replaces "**"
).join('|') + ')$'; // replaces ","
config.reIgnoreOperation = RegExp(`${ignoreSuffix}|${ignorePath}`);
diff --git a/src/plugins/AxiosPlugin.ts b/src/plugins/AxiosPlugin.ts
index 7a1ac28..9612abc 100644
--- a/src/plugins/AxiosPlugin.ts
+++ b/src/plugins/AxiosPlugin.ts
@@ -21,7 +21,6 @@
import { URL } from 'url';
import ContextManager from '../trace/context/ContextManager';
import { Component } from '../trace/Component';
-import Span from '../trace/span/Span';
import Tag from '../Tag';
import { SpanLayer } from '../proto/language-agent/Tracing_pb';
import { createLogger } from '../logging';
@@ -37,62 +36,15 @@
if (logger.isDebugEnabled()) {
logger.debug('installing axios plugin');
}
- const axios = installer.require('axios').default;
- this.interceptClientRequest(axios);
+
+ this.interceptClientRequest(installer);
}
- private interceptClientRequest(axios: any) {
- const copyStatusAndStop = (span: Span, response: any) => {
- if (response) {
- if (response.status) {
- span.tag(Tag.httpStatusCode(response.status));
- }
- if (response.statusText) {
- span.tag(Tag.httpStatusMsg(response.statusText));
- }
- }
+ private interceptClientRequest(installer: PluginInstaller): void {
+ const defaults = installer.require('axios/lib/defaults');
+ const defaultAdapter = defaults.adapter; // this will be http adapter
- span.stop();
- };
-
- 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;
- });
-
- return config;
- },
-
- (error: any) => {
- error.config.span.error(error);
- error.config.span.stop();
-
- return Promise.reject(error);
- },
- );
-
- axios.interceptors.response.use(
- (response: any) => {
- copyStatusAndStop(response.config.span, response);
-
- return response;
- },
-
- (error: any) => {
- error.config.span.error(error);
-
- copyStatusAndStop(error.config.span, error.response);
-
- return Promise.reject(error);
- },
- );
-
- const _request = axios.Axios.prototype.request;
-
- axios.Axios.prototype.request = function(config: any) {
+ defaults.adapter = (config: any) => {
const { host, pathname: operation } = new URL(config.url); // TODO: this may throw invalid URL
const span = ContextManager.current.newExitSpan(operation, host).start();
@@ -101,16 +53,50 @@
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 });
+ span.inject().items.forEach((item) => {
+ config.headers[item.key] = item.value;
+ });
+
+ const copyStatusAndStop = (response: any) => {
+ if (response) {
+ if (response.status) {
+ span.tag(Tag.httpStatusCode(response.status));
+ if (response.status >= 400) {
+ span.errored = true;
+ }
+ }
+
+ if (response.statusText) {
+ span.tag(Tag.httpStatusMsg(response.statusText));
+ }
+ }
+
+ span.stop();
+ };
+
+ return defaultAdapter(config).then(
+ (response: any) => {
+ copyStatusAndStop(response);
+
+ return response;
+ },
+
+ (error: any) => {
+ span.error(error);
+ copyStatusAndStop(error.response);
+
+ return Promise.reject(error);
+ }
+ );
+
} catch (e) {
span.error(e);
span.stop();
throw e;
}
- };
+ }
}
}
diff --git a/src/plugins/HttpPlugin.ts b/src/plugins/HttpPlugin.ts
index e679ec9..ca01811 100644
--- a/src/plugins/HttpPlugin.ts
+++ b/src/plugins/HttpPlugin.ts
@@ -27,6 +27,8 @@
import { SpanLayer } from '../proto/language-agent/Tracing_pb';
import { ContextCarrier } from '../trace/context/ContextCarrier';
+const NativePromise = (async () => null)().constructor; // may be different from globally overridden Promise
+
class HttpPlugin implements SwPlugin {
readonly module = 'http';
readonly versions = '*';
@@ -75,30 +77,33 @@
span.tag(Tag.httpURL(httpURL));
}
- const request: ClientRequest = _request.apply(this, arguments);
+ const req: ClientRequest = _request.apply(this, arguments);
span.inject().items.forEach((item) => {
- request.setHeader(item.key, item.value);
+ req.setHeader(item.key, item.value);
});
- request.on('close', stopIfNotStopped);
- request.on('abort', () => (span.errored = true, stopIfNotStopped()));
- request.on('error', (err) => (span.error(err), stopIfNotStopped()));
+ req.on('close', stopIfNotStopped);
+ req.on('abort', () => (span.errored = true, stopIfNotStopped()));
+ req.on('error', (err) => (span.error(err), stopIfNotStopped()));
- request.prependListener('response', (res) => {
+ req.prependListener('response', (res) => {
span.resync();
span.tag(Tag.httpStatusCode(res.statusCode));
+
if (res.statusCode && res.statusCode >= 400) {
span.errored = true;
}
if (res.statusMessage) {
span.tag(Tag.httpStatusMsg(res.statusMessage));
}
+
+ res.on('end', stopIfNotStopped);
});
span.async();
- return request;
+ return req;
} catch (e) {
if (!stopped) { // don't want to set error if exception occurs after clean close
@@ -112,45 +117,74 @@
}
private interceptServerRequest(module: any) {
- const _emit = module.Server.prototype.emit;
+ /// TODO? full event protocol support not currently implemented (prependListener(), removeListener(), etc...)
+ const _addListener = module.Server.prototype.addListener;
- module.Server.prototype.emit = function () {
- if (arguments[0] !== 'request') {
- return _emit.apply(this, arguments);
- }
+ module.Server.prototype.addListener = module.Server.prototype.on = function (event: any, handler: any, ...addArgs: any[]) {
+ return _addListener.call(this, event, event === 'request' ? _sw_request : handler, ...addArgs);
- const [req, res] = [arguments[1] as IncomingMessage, arguments[2] as ServerResponse];
+ function _sw_request(this: any, req: IncomingMessage, res: ServerResponse, ...reqArgs: any[]) {
+ const headers = req.rawHeaders || [];
+ const headersMap: { [key: string]: string } = {};
- const headers = req.rawHeaders || [];
- const headersMap: { [key: string]: string } = {};
-
- for (let i = 0; i < headers.length / 2; i += 2) {
- headersMap[headers[i]] = headers[i + 1];
- }
-
- const carrier = ContextCarrier.from(headersMap);
- const operation = (req.url || '/').replace(/\?.*/g, '');
- const span = ContextManager.current.newEntrySpan(operation, carrier);
-
- 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));
-
- 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));
+ for (let i = 0; i < headers.length / 2; i += 2) {
+ headersMap[headers[i]] = headers[i + 1];
}
- return ret;
+ const carrier = ContextCarrier.from(headersMap);
+ const operation = (req.url || '/').replace(/\?.*/g, '');
+ const span = ContextManager.current.newEntrySpan(operation, carrier).start();
- }, this, arguments);
+ const copyStatusAndStop = () => {
+ span.tag(Tag.httpStatusCode(res.statusCode));
+ if (res.statusCode && res.statusCode >= 400) {
+ span.errored = true;
+ }
+ if (res.statusMessage) {
+ span.tag(Tag.httpStatusMsg(res.statusMessage));
+ }
+
+ span.stop();
+ };
+
+ try {
+ span.component = Component.HTTP_SERVER;
+ span.layer = SpanLayer.HTTP;
+ span.peer = req.connection.remoteFamily === 'IPv6'
+ ? `[${req.connection.remoteAddress}]:${req.connection.remotePort}`
+ : `${req.connection.remoteAddress}:${req.connection.remotePort}`;
+ span.tag(Tag.httpURL((req.headers.host || '') + req.url));
+
+ let ret = handler.call(this, req, res, ...reqArgs);
+ const type = ret?.constructor;
+
+ if (type !== Promise && type !== NativePromise) {
+ copyStatusAndStop();
+
+ } else {
+ ret = ret.then((r: any) => {
+ copyStatusAndStop();
+
+ return r;
+ },
+
+ (error: any) => {
+ span.error(error);
+ span.stop();
+
+ return Promise.reject(error);
+ })
+ }
+
+ return ret;
+
+ } catch (e) {
+ span.error(e);
+ span.stop();
+
+ throw e;
+ }
+ }
};
}
}
diff --git a/tests/plugins/axios/client.ts b/tests/plugins/axios/client.ts
index 91f435d..bcc3308 100644
--- a/tests/plugins/axios/client.ts
+++ b/tests/plugins/axios/client.ts
@@ -26,8 +26,8 @@
maxBufferSize: 1000,
});
-const server = http.createServer((req, res) => {
- axios
+const server = http.createServer(async (req, res) => {
+ await axios
.get(`http://${process.env.SERVER || 'localhost:5000'}${req.url}`)
.then((r) => res.end(JSON.stringify(r.data)))
.catch(err => res.end(JSON.stringify(err)));
diff --git a/tests/plugins/axios/expected.data.yaml b/tests/plugins/axios/expected.data.yaml
index fb28cf4..3ecd711 100644
--- a/tests/plugins/axios/expected.data.yaml
+++ b/tests/plugins/axios/expected.data.yaml
@@ -21,16 +21,42 @@
segments:
- segmentId: not null
spans:
+ - operationName: /json
+ operationId: 0
+ parentSpanId: 0
+ spanId: 1
+ spanLayer: Http
+ startTime: gt 0
+ endTime: gt 0
+ componentId: 4005
+ spanType: Exit
+ peer: httpbin.org
+ skipAnalysis: false
+ tags:
+ - key: http.url
+ value: httpbin.org:80/json
+ - key: http.status.code
+ value: '200'
+ - key: http.status.msg
+ value: OK
- operationName: /axios
operationId: 0
parentSpanId: -1
spanId: 0
spanLayer: Http
+ startTime: gt 0
+ endTime: gt 0
+ componentId: 49
+ spanType: Entry
+ peer: not null
+ skipAnalysis: false
tags:
- key: http.url
value: server:5000/axios
- key: http.status.code
value: '200'
+ - key: http.status.msg
+ value: OK
refs:
- parentEndpoint: ''
networkAddress: server:5000
@@ -40,20 +66,19 @@
parentServiceInstance: not null
parentService: client
traceId: not null
- startTime: gt 0
- endTime: gt 0
- componentId: 49
- spanType: Entry
- peer: server:5000
- skipAnalysis: false
- - operationName: /json
+ - serviceName: client
+ segmentSize: 1
+ segments:
+ - segmentId: not null
+ spans:
+ - operationName: /axios
operationId: 0
parentSpanId: 0
spanId: 1
spanLayer: Http
tags:
- key: http.url
- value: httpbin.org:80/json
+ value: server:5000/axios
- key: http.status.code
value: '200'
- key: http.status.msg
@@ -62,13 +87,8 @@
endTime: gt 0
componentId: 4005
spanType: Exit
- peer: httpbin.org
+ peer: server:5000
skipAnalysis: false
- - serviceName: client
- segmentSize: 1
- segments:
- - segmentId: not null
- spans:
- operationName: /axios
operationId: 0
parentSpanId: -1
@@ -79,27 +99,11 @@
value: localhost:5001/axios
- key: http.status.code
value: '200'
- startTime: gt 0
- endTime: gt 0
- componentId: 49
- 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
+ componentId: 49
+ spanType: Entry
+ peer: not null
skipAnalysis: false
diff --git a/tests/plugins/express/expected.data.yaml b/tests/plugins/express/expected.data.yaml
index 9cf0fad..009c233 100644
--- a/tests/plugins/express/expected.data.yaml
+++ b/tests/plugins/express/expected.data.yaml
@@ -21,6 +21,24 @@
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: /express
operationId: 0
parentSpanId: -1
@@ -48,24 +66,6 @@
spanType: Entry
peer: server:5000
skipAnalysis: false
- - 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
- serviceName: client
segmentSize: 1
segments:
@@ -73,24 +73,6 @@
spans:
- operationName: /express
operationId: 0
- parentSpanId: -1
- spanId: 0
- spanLayer: Http
- tags:
- - key: http.url
- value: localhost:5001/express
- - key: http.status.code
- value: '200'
- - key: http.status.msg
- value: OK
- startTime: gt 0
- endTime: gt 0
- componentId: 4002
- spanType: Entry
- peer: localhost:5001
- skipAnalysis: false
- - operationName: /express
- operationId: 0
parentSpanId: 0
spanId: 1
spanLayer: Http
@@ -107,3 +89,21 @@
spanType: Exit
peer: server:5000
skipAnalysis: false
+ - operationName: /express
+ operationId: 0
+ parentSpanId: -1
+ spanId: 0
+ spanLayer: Http
+ tags:
+ - key: http.url
+ value: localhost:5001/express
+ - key: http.status.code
+ value: '200'
+ - key: http.status.msg
+ value: OK
+ startTime: gt 0
+ endTime: gt 0
+ componentId: 4002
+ spanType: Entry
+ peer: not null
+ skipAnalysis: false
diff --git a/tests/plugins/http/expected.data.yaml b/tests/plugins/http/expected.data.yaml
index dfb19ac..b498ff7 100644
--- a/tests/plugins/http/expected.data.yaml
+++ b/tests/plugins/http/expected.data.yaml
@@ -30,7 +30,7 @@
endTime: gt 0
componentId: 49
spanType: Entry
- peer: server:5000
+ peer: not null
skipAnalysis: false
tags:
- key: http.url
@@ -78,7 +78,7 @@
endTime: gt 0
componentId: 49
spanType: Entry
- peer: localhost:5001
+ peer: not null
skipAnalysis: false
tags:
- key: http.url
