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

fix: connected deferred exit parent span with original entry span #1346

Merged
merged 5 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ app.get('/request-url-only', (req, res) => {
httpModule.request(createUrl(req, '/request-only-url'), () => res.sendStatus(200)).end();
});

app.get('/request-deferred', (req, res) => {
setTimeout(() => {
httpModule.get('http://example.com?k=2', () => {}).end();
}, 500);

httpModule.get('http://example.com?k=1', () => res.sendStatus(200)).end();
});

app.get('/request-options-only', (req, res) => {
const downStreamQuery = {};
if (req.query.withQuery) {
Expand Down
56 changes: 46 additions & 10 deletions packages/collector/test/tracing/protocols/http/client/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,51 @@ function registerTests(useHttps) {
await clientControls.clearIpcMessages();
});

// HTTP requests can be triggered via http.request(...) + request.end(...) or http.get(...).
// Both http.request and http.get accept
// - an URL, an options object and a callback
// - only an URL and a callback, or
// - only an options object (containing the parts of the URL) and a callback.
// The URL can be a string or an URL object.
//
// This following tests cover all variants.
it('must trace request in background', () => {
return clientControls
.sendRequest({
method: 'GET',
path: '/request-deferred'
})
.then(() => {
return retry(() => {
return globalAgent.instance.getSpans().then(spans => {
const entryInClient = verifyRootHttpEntry({
spans,
host: `localhost:${clientControls.getPort()}`,
url: '/request-deferred'
});

[false, true].forEach(urlObject => {
verifyHttpExit({
spans,
parent: entryInClient,
url: 'http://example.com/',
params: 'k=1'
});

verifyHttpExit({
spans,
parent: entryInClient,
url: 'http://example.com/',
params: 'k=2'
});
});
});
});
});

[
// HTTP requests can be triggered via http.request(...) + request.end(...) or http.get(...).
// Both http.request and http.get accept
// - an URL, an options object and a callback
// - only an URL and a callback, or
// - only an options object (containing the parts of the URL) and a callback.
// The URL can be a string or an URL object.
//
// This following tests cover all variants.

(false, true)
].forEach(urlObject => {
[false, true].forEach(withQuery => {
const urlParam = urlObject ? 'urlObject' : 'urlString';

Expand Down Expand Up @@ -846,7 +881,7 @@ function verifyHttpEntry({ spans, parent, host, url = '/', method = 'GET', statu
return expectExactlyOneMatching(spans, expectations);
}

function verifyHttpExit({ spans, parent, url = '/', method = 'GET', status = 200, synthetic = false }) {
function verifyHttpExit({ spans, parent, url = '/', method = 'GET', status = 200, synthetic = false, params = null }) {
return expectExactlyOneMatching(spans, [
span => expect(span.n).to.equal('node.http.client'),
span => expect(span.k).to.equal(constants.EXIT),
Expand All @@ -856,6 +891,7 @@ function verifyHttpExit({ spans, parent, url = '/', method = 'GET', status = 200
span => expect(span.data.http.url).to.equal(url),
span => expect(span.data.http.method).to.equal(method),
span => expect(span.data.http.status).to.equal(status),
span => (params ? expect(span.data.http.params).to.equal(params) : expect(span.data.http.params).to.not.exist),
span => (!synthetic ? expect(span.sy).to.not.exist : expect(span.sy).to.be.true)
]);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ app.use(bodyParser.json());

app.get('/', (req, res) => res.sendStatus(200));

app.get('/fetch-deferred', async (req, res) => {
setTimeout(async () => {
await fetch('http://example.com?k=2');
}, 500);

await fetch('http://example.com?k=1');
res.sendStatus(200);
});

app.get('/fetch', async (req, res) => {
const resourceArgument = createResourceArgument(req, '/fetch');
let response;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,39 @@ mochaSuiteFn('tracing/native fetch', function () {
await clientControls.clearIpcMessages();
});

it('must trace request in background', () => {
return clientControls
.sendRequest({
method: 'GET',
path: '/fetch-deferred'
})
.then(() => {
return retry(() => {
return globalAgent.instance.getSpans().then(spans => {
const entryInClient = verifyRootHttpEntry({
spans,
host: `localhost:${clientControls.getPort()}`,
url: '/fetch-deferred'
});

verifyHttpExit({
spans,
parent: entryInClient,
url: 'http://example.com/',
params: 'k=1'
});

verifyHttpExit({
spans,
parent: entryInClient,
url: 'http://example.com/',
params: 'k=2'
});
});
});
});
});

// See https://developer.mozilla.org/en-US/docs/Web/API/fetch#parameters.

describe('capture attributes from different resource types', () => {
Expand Down Expand Up @@ -689,7 +722,8 @@ function verifyHttpExit({
withClientError,
withServerError,
withTimeout,
serverControls
serverControls,
params = null
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added another optional check.
If the test passes params, we test params.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it!

}) {
const expectations = [
span => expect(span.n).to.equal('node.http.client'),
Expand All @@ -700,6 +734,7 @@ function verifyHttpExit({
span => expect(span.ec).to.equal(withClientError || withServerError || withTimeout ? 1 : 0),
span => expect(span.data.http.url).to.equal(url),
span => expect(span.data.http.method).to.equal(method),
span => (params ? expect(span.data.http.params).to.equal(params) : expect(span.data.http.params).to.not.exist),
span => expect(span.sy).to.not.exist
];
if (withClientError) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ function instrument(coreModule, forceHttps) {
}

cls.ns.run(() => {
const span = cls.startSpan('node.http.client', constants.EXIT);
const span = cls.startSpan('node.http.client', constants.EXIT, parentSpan.t, parentSpan.s);

// startSpan updates the W3C trace context and writes it back to CLS, so we have to refetch the updated context
// object from CLS.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,12 @@ function instrument() {
// eslint-disable-next-line no-unused-vars
let w3cTraceContext = cls.getW3cTraceContext();

const skipTracingResult = cls.skipExitTracing({ isActive, extendedResponse: true, skipParentSpanCheck: true });
const skipTracingResult = cls.skipExitTracing({
isActive,
extendedResponse: true,
skipParentSpanCheck: true,
skipIsTracing: true
});

// If there is no active entry span, we fall back to the reduced span of the most recent entry span. See comment in
// packages/core/src/tracing/clsHooked/unset.js#storeReducedSpan.
Expand All @@ -100,7 +105,7 @@ function instrument() {
}

return cls.ns.runAndReturn(() => {
const span = cls.startSpan('node.http.client', constants.EXIT);
const span = cls.startSpan('node.http.client', constants.EXIT, parentSpan.t, parentSpan.s);

// startSpan updates the W3C trace context and writes it back to CLS, so we have to refetch the updated context
w3cTraceContext = cls.getW3cTraceContext();
Expand Down