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

FastifyDeprecation warning for accessing request.routerPath when accessing a non-existing page #1757

Closed
omBratteng opened this issue Oct 29, 2023 · 17 comments · Fixed by #1763
Assignees
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization

Comments

@omBratteng
Copy link

Separating the issue reported here #1275 (comment)

What version of OpenTelemetry are you using?

@opentelemetry/instrumentation-fastify@032.3

What version of Node are you using?

v18

What did you do?

Instrumented node with fastify instrumentation, did a request to a non-existing page.

Can be reproduced by starting the fastify example and doing a request to any non-existent page

curl http://localhost:8080/i_do_not_exist

What did you expect to see?

No deprecation notices

What did you see instead?

(node:9) [FSTDEP017] FastifyDeprecation: You are accessing the deprecated "request.routerPath" property. Use "request.routeOptions.url" instead. Property "req.routerPath" will be removed in `fastify@5`.

Additional context

We are using the latest version of the intrumentation

https://github.com/dailydotdev/daily-api/blob/dc3bbecd7c04984a60a224b19ae9752102caf09e/package.json#L48

and version 4.24.2 of fastify

https://github.com/dailydotdev/daily-api/blob/dc3bbecd7c04984a60a224b19ae9752102caf09e/package.json#L71

So digging a bit further into it, I think it might when the requests are to not found pages. but then both request.routeOptions.url and request.routerPath are undefined.

@Wazbat
Copy link

Wazbat commented Oct 29, 2023

I'm also getting this when accessing existing pages, is that perhaps an unrelated issue?

See #1757 (comment)

@omBratteng
Copy link
Author

That might be related to #1275 which should have been fixed in #1679

@dyladan
Copy link
Member

dyladan commented Nov 1, 2023

I'll cut a release that includes #1679 and we can see if this is fixed. Seems to me that it shoudl be.

@10xLaCroixDrinker
Copy link
Contributor

@dyladan That says it was released 3 weeks ago. Was it not?

@dyladan
Copy link
Member

dyladan commented Nov 1, 2023

Sorry I didn't confirm before typing that. @Wazbat can you confirm your issue is fixed?

Seems we didn't catch all uses of deprecated properties then

@trentm
Copy link
Contributor

trentm commented Nov 1, 2023

@dyladan I can take this. I was just looking at it.

@omBratteng
Copy link
Author

I think the issue is that on 404 pages, neither request.routeOptions.url or the deprecated request.routerPath is available. Could maybe check if request.is404 is false and then set the path?

@Wazbat
Copy link

Wazbat commented Nov 1, 2023

Sorry I didn't confirm before typing that. @Wazbat can you confirm your issue is fixed?

I'm using the following packages and versions:

{
    "@autotelic/fastify-opentelemetry": "^0.17.1",
    "@opentelemetry/instrumentation": "^0.44.0",
    "@opentelemetry/instrumentation-fastify": "^0.32.3",
    "@opentelemetry/instrumentation-http": "^0.44.0",
    "@opentelemetry/resources": "^1.17.1",
    "@opentelemetry/sdk-trace-node": "^1.17.0",
    "fastify": "^4.24.3",
}

And simplified app is as follows

import Fastify from 'fastify';
import openTelemertryPlugin from '@autotelic/fastify-opentelemetry';
const fastify = Fastify();
await fastify.register(openTelemertryPlugin, {
    wrapRoutes: ['/my/route']
});
fastify.post('/my/route', async (req, res) =>{
    const { activeSpan } = req.openTelemetry();
    activeSpan?.setAttributes({ key: 'value' });
    res.send('whatever');
});

When running my application with this, I'm seeing these logs

(node:1) [FSTDEP017] FastifyDeprecation: You are accessing the deprecated "request.routerPath" property. Use "request.routeOptions.url" instead. Property "req.routerPath" will be removed in `fastify@5`.

@omBratteng
Copy link
Author

@Wazbat it's the @autotelic/fastify-opentelemetry plugin that is using the routerPath
https://github.com/autotelic/fastify-opentelemetry/blob/41dfdb788acbb83733052cf0cfe5060e8ad677ae/index.js#L14-L15

@trentm
Copy link
Contributor

trentm commented Nov 1, 2023

Could maybe check if request.is404 is false and then set the path?

I'd prefer not to have different logic to try to sniff if the new attributes are the appropriate ones to use. I saw some reference about undefined values for a Fastify error as well. My suspicion is that cases other than a 404 might have an empty request.routeOptions.url.

request.routeOptions was added in fastify/fastify#4397 and it included .url from its inception. So I'm planning to switch on if the request.routeOptions property exists.


Currently I'm adding a 404 test case and there is a separate weirdness. The fastify.name property is added to the span, but it is assuming all handlers are user-provided handlers, and not built-in ones:

      const handlerName = handler?.name.substr(6);

There it is assuming a handler is of the form bound whateverUserHandlerFunctionName. However, for a 404 the handler.name is basic404: https://github.com/fastify/fastify/blob/396b8b95bf0f1313b9deeb387d68fdc7ffa97abe/lib/fourOhFour.js#L48

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this issue Nov 1, 2023
…ion warning for 404 request

For a 404 `request.routeOptions.url` is undefined. Since fastify@4.10.0
when routeOptions was added, we shouldn't fallback to the deprecated
request.routerPath.

This also corrects the assumption that the handler name is "bound ..."
in all cases. E.g. for a 404 it is Fastify's core "basic404" internal
function.

Fixes: open-telemetry#1757
@Wazbat
Copy link

Wazbat commented Nov 2, 2023

@Wazbat it's the @autotelic/fastify-opentelemetry plugin that is using the routerPath https://github.com/autotelic/fastify-opentelemetry/blob/41dfdb788acbb83733052cf0cfe5060e8ad677ae/index.js#L14-L15

Ah thanks for clarifying!

@10xLaCroixDrinker
Copy link
Contributor

@Wazbat I'll open a PR to autotelic/fastify-opentelemetry

@Wazbat
Copy link

Wazbat commented Nov 2, 2023

@Wazbat I'll open a PR to autotelic/fastify-opentelemetry

Thank you! That'd be awesome. My workaround for now is as follows, should anyone find this through google:

await fastify.register(openTelemertryPlugin, {
    wrapRoutes: ['/v3/search/merchant'],
    formatSpanName: (req) => (req.routeOptions?.url ? `${req.method} ${req.routeOptions.url}` : req.method)
});

Looks like this GH issue is unrelated, please disregard my comments about it occuring on existing routes

@Fernandomr88
Copy link

Not sure if this contributes to this issue, but that log appears on more cases than just accessing non-existing pages.

I have been using AppSignal (which depends on @opentelemetry/instrumentation-fastify) and my console is flooded of these logs. Not sure which events triggers this.

Btw, using NestJS.

[FSTDEP017] FastifyDeprecation: You are accessing the deprecated "request.routerPath" property. Use "request.routeOptions.url" instead. Property "req.routerPath" will be removed in fastify@5.

image

trentm added a commit that referenced this issue Nov 29, 2023
… FastifyDeprecation warning for 404 request (#1763)

For a 404 `request.routeOptions.url` is undefined. Since fastify@4.10.0
when routeOptions was added, we shouldn't fallback to the deprecated
request.routerPath.

This also corrects the assumption that the handler name is "bound ..."
in all cases. E.g. for a 404 it is Fastify's core "basic404" internal
function.

Also add a test that Fastify instrumentation works for ESM usage.

Fixes: #1757
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
@Fernandomr88
Copy link

@trentm does that fix apply only to non-existing routes/pages?

@trentm
Copy link
Contributor

trentm commented Nov 29, 2023

does that fix apply only to non-existing routes/pages?

@Fernandomr88 With that commit, the instrumentation-fastify will no longer access request.routerPath in versions of fastify that have request.routeOptions.* (that is fastify >=4.10.0). So, this should prevent the [FSTDEP017] FastifyDeprecation: You are accessing the deprecated "request.routerPath" property. warning in any cases -- including in certain error response cases other than a 404 that I mentioned at #1757 (comment) Note that I haven't added a test case for cases other than a 404.

@omBratteng
Copy link
Author

Updated and deployed, verified that we don't get this deprecation warning anymore
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p4 Bugs and spec inconsistencies which do not fall into a higher prioritization
Projects
None yet
7 participants