Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
18 changes: 18 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
"fastify":
- versions: "4.23.2"
# Sanity check the first 4.x release, instead of all releases, plus recent
# releases.
- versions: "4.0.0 || >=4.24.3 <5"
commands: npm run test

# Fastify versions after 4.18.0 require a typescript greater than 4.4.4.
"typescript":
- versions: "4.7.4"
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,19 @@
"@fastify/express": "^2.0.2",
"@opentelemetry/api": "^1.3.0",
"@opentelemetry/context-async-hooks": "^1.8.0",
"@opentelemetry/contrib-test-utils": "^0.35.0",
"@opentelemetry/instrumentation-http": "^0.45.1",
"@opentelemetry/sdk-trace-base": "^1.8.0",
"@opentelemetry/sdk-trace-node": "^1.8.0",
"@types/express": "4.17.18",
"@types/mocha": "7.0.2",
"@types/node": "18.15.3",
"@types/semver": "7.5.5",
"fastify": "4.18.0",
"mocha": "7.2.0",
"nyc": "15.1.0",
"rimraf": "5.0.5",
"semver": "^7.5.4",
"test-all-versions": "5.0.1",
"ts-mocha": "10.0.0",
"typescript": "4.4.4"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ export class FastifyInstrumentation extends InstrumentationBase {
const anyRequest = request as any;

const rpcMetadata = getRPCMetadata(context.active());
const routeName =
anyRequest.routeOptions?.config?.url || request.routerPath;
const routeName = anyRequest.routeOptions
? anyRequest.routeOptions.url // since [email protected]
: request.routerPath;
Copy link
Contributor Author

@trentm trentm Nov 6, 2023

Choose a reason for hiding this comment

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

Regarding the codecov warning on this line and below: TAV tests could cover line 101, but currently they won't because it doesn't go back to an early-enough fastify version. Shall I add an // istanbul ignore next directive here?

Update: Actually, perhaps an istanbul directive won't help, because that is what nyc is using. I don't know what codecov is using. When I run the tests locally, I get different lines that are uncovered:

---------------------|---------|----------|---------|---------|-------------------
File                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------|---------|----------|---------|---------|-------------------
All files            |   93.91 |    75.24 |   94.73 |   93.91 |
 src                 |   93.43 |    73.68 |   94.28 |   93.43 |
  constants.ts       |     100 |      100 |     100 |     100 |
  instrumentation.ts |   96.07 |       80 |     100 |   96.07 | 153-157,161,235
  utils.ts           |   84.84 |       56 |   71.42 |   84.84 | 107-113,119
 src/enums           |     100 |      100 |     100 |     100 |
  AttributeNames.ts  |     100 |      100 |     100 |     100 |
---------------------|---------|----------|---------|---------|-------------------

Copy link
Member

Choose a reason for hiding this comment

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

Interesting; AFAIK codecov uses the reports generated by istanbul, so adding the directive should help. Looks like the uncovered lines above are actually not tested at the moment (see full file on the codecov report - not sure if you have access). We could also add an earlier fastify version to TAV if that fixes the issue.

I've just recently added TAV to this package, having more versions tested would likely be a good idea anyway.

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've added testing of [email protected] plus the current latest and any subsequent fastify 4.x releases to the .tav.yml.
I don't know if coverage data from TAV runs get included in the codecov summary.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, it looks like reports from TAV are not included.

IMO just having the version tested is good enough for now, we may look into adding coverage from TAV in the future, but we've been having problems with codecov for a while now, so I think this will likely be a larger task.

if (routeName && rpcMetadata?.type === RPCType.HTTP) {
rpcMetadata.route = routeName;
}
Expand Down Expand Up @@ -265,18 +266,21 @@ export class FastifyInstrumentation extends InstrumentationBase {
const anyRequest = request as any;

const handler =
anyRequest.routeOptions?.handler || anyRequest.context?.handler || {};
anyRequest.routeOptions?.handler || anyRequest.context?.handler;

const handlerName = handler?.name.substr(6);
const handlerName = handler?.name.startsWith('bound ')
? handler.name.substr(6)
: handler?.name;
const spanName = `${FastifyNames.REQUEST_HANDLER} - ${
handlerName || this.pluginName || ANONYMOUS_NAME
}`;

const spanAttributes: SpanAttributes = {
[AttributeNames.PLUGIN_NAME]: this.pluginName,
[AttributeNames.FASTIFY_TYPE]: FastifyTypes.REQUEST_HANDLER,
[SemanticAttributes.HTTP_ROUTE]:
anyRequest.routeOptions?.config?.url || request.routerPath,
[SemanticAttributes.HTTP_ROUTE]: anyRequest.routeOptions
? anyRequest.routeOptions.url // since [email protected]
: request.routerPath,
};
if (handlerName) {
spanAttributes[AttributeNames.FASTIFY_NAME] = handlerName;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Copyright The OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

// Use fastify from an ES module:
// node --experimental-loader=@opentelemetry/instrumentation/hook.mjs use-fastify.mjs

import { trace } from '@opentelemetry/api';
import { createTestNodeSdk } from '@opentelemetry/contrib-test-utils';

import { FastifyInstrumentation } from '../../build/src/index.js';

const sdk = createTestNodeSdk({
serviceName: 'use-fastify',
instrumentations: [
new FastifyInstrumentation()
]
})
sdk.start();

import Fastify from 'fastify';
import http from 'http';

// Start a fastify server.
const app = Fastify();
app.get('/a-route', function aRoute(_request, reply) {
reply.send({ hello: 'world' });
})
const addr = await app.listen({ port: 0 });

// Make a single request to it.
await new Promise(resolve => {
http.get(addr + '/a-route', (res) => {
res.resume();
res.on('end', () => {
resolve();
});
})
})

await app.close();
await sdk.shutdown();
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,21 @@ import {
SimpleSpanProcessor,
} from '@opentelemetry/sdk-trace-base';
import { Span } from '@opentelemetry/api';
import {
getPackageVersion,
runTestFixture,
TestCollector,
} from '@opentelemetry/contrib-test-utils';
import * as semver from 'semver';
import * as http from 'http';
import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
import { AttributeNames, FastifyInstrumentation } from '../src';
import { FastifyRequestInfo } from '../src/types';

const URL = require('url').URL;

const fastifyVersion = getPackageVersion('fastify');

const httpRequest = {
get: (options: http.ClientRequestArgs | string) => {
return new Promise((resolve, reject) => {
Expand Down Expand Up @@ -183,6 +191,23 @@ describe('fastify', () => {
assert.strictEqual(span.parentSpanId, baseSpan.spanContext().spanId);
});

it('should generate span for 404 request', async () => {
await startServer();
await httpRequest.get(`http://localhost:${PORT}/no-such-route`);

const spans = memoryExporter.getFinishedSpans();
assert.strictEqual(spans.length, 5);
const span = spans[2];
assert.deepStrictEqual(span.attributes, {
'fastify.name': 'basic404',
'fastify.type': 'request_handler',
'plugin.name': 'fastify -> @fastify/express',
});
assert.strictEqual(span.name, 'request handler - basic404');
const baseSpan = spans[1];
assert.strictEqual(span.parentSpanId, baseSpan.spanContext().spanId);
});

describe('when subsystem is registered', () => {
beforeEach(async () => {
httpInstrumentation.enable();
Expand Down Expand Up @@ -424,12 +449,17 @@ describe('fastify', () => {
await startServer();
});

it('preClose is not instrumented', async () => {
app.addHook('preClose', () => {
assertRootContextActive();
});
it('preClose is not instrumented', async function () {
// 'preClose' was added in [email protected].
if (semver.lt(fastifyVersion, '4.16.0')) {
this.skip();
} else {
app.addHook('preClose', () => {
assertRootContextActive();
});

await startServer();
await startServer();
}
});

it('onClose is not instrumented', async () => {
Expand Down Expand Up @@ -507,4 +537,30 @@ describe('fastify', () => {
});
});
});

it('should work with ESM usage', async () => {
await runTestFixture({
cwd: __dirname,
argv: ['fixtures/use-fastify.mjs'],
env: {
NODE_OPTIONS:
'--experimental-loader=@opentelemetry/instrumentation/hook.mjs',
NODE_NO_WARNINGS: '1',
},
checkResult: (err, stdout, stderr) => {
assert.ifError(err);
},
checkCollector: (collector: TestCollector) => {
const spans = collector.sortedSpans;
assert.strictEqual(spans.length, 1);
assert.strictEqual(spans[0].name, 'request handler - aRoute');
assert.strictEqual(
spans[0].attributes.filter(a => a.key === 'plugin.name')[0]?.value
?.stringValue,
'fastify',
'attribute plugin.name'
);
},
});
});
});