Access handler name

[SimenB]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the feature has not already been requested

🚀 Feature Proposal

Open Telemetry accesses the (now deprecated via #4216) request.context property so it can try to get the name of the handler function. See https://github.com/open-telemetry/opentelemetry-js-contrib/blob/4cafe6becc19cfb76c53393cb03a5c0574a68b0d/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts#L264-L268

With request.context deprecated, we now get a big deprecation warning printed every the app gets its first request. Would it be possible to expose this information in a meaningful way?

There's an issue in their repo as well: open-telemetry/opentelemetry-js-contrib#1275

Motivation

It's useful to get the name of a handler function as a span name in traces.

Example

Would replace request.context.handler.name

[climba03003]

Would you like to send a PR to expose the handler in request.routeOptions?
It is a separate object that would not mutate the #context, routes. And should not have any side-effect at all.

It should be good enough to replace the original usage.

Note: I though function is not useful for exposing, it clearly not.

[Eomm]

As we have an instance.pluginName we could add a request.routeOptions.routeName

[jsumners]

The name of the route isn't really an "option" though, is it? It's entirely possible that it should have been:

route.info = {
  handler,
  options
}

Which would give route.info.handler.name.

[cesarvspr]

anyone working on to expose the handler name on the request.routeOptions.routeName? otherwise I can do it

[Uzlopak]

I see that you are eager to participate. The issue has to be discussed first. Imho this is a good-first-issue, but until we didnt come to a conclusion, it should not be started.

Imho: You can always participate in discussions. By understanding the arguments, providing your own arguments and helping to find a good solution, you will have a greater impact.

I also tend to jsumners proposer solution.

[james-gardner]

The name of the route isn't really an "option" though, is it? It's entirely possible that it should have been:

route.info = {
  handler,
  options
}

Which would give route.info.handler.name.

I get what you mean. What about exposing it from routeConfig or will that have side-effects?

[jsumners]

I get what you mean. What about exposing it from routeConfig or will that have side-effects?

Yes. request.routeConfig is the object the user has passed in as their own configuration:

fastify.route({
  path: '/foo',
  method: 'GET',
  config: {
    // this object is returned by `request.routeConfig`
    foo: 'foo'
  }
  handler
}

async function handler (request, reply) {
  return request.routeConfig
}

What I'm saying is that we made a mistake in #4216 in naming the new property request.routeOptions. Unfortunately, I think that means we need to immediately deprecate that property with a notice to use request.routeInfo.options instead. Then this PR can be solved by also adding request.routeInfo.handler.

I'm not sure how others in @fastify/core feel about that though.

[Uzlopak]

What about request.route? Is that already used?

[metcoder95]

What I'm saying is that we made a mistake in #4216 in naming the new property request.routeOptions.

Are you referring to #4397? The one you mention(#4216) adds the routeConfig property

Is there a strong opinion towards calling it routeContext? My rationale is that what we are kinda trying to expose through this new property is the context itself about the route, what's the name of the handler, and all the options used to set up the route among some other important configuration in the context of the route defined.

We are in the process of totally deprecating Request#context in favor of Request#routeConfig for the custom configuration passed so in theory the concept and naming are somehow free.

[jsumners]

Are you referring to #4397?

🤦‍♂️ yes.

Is there a strong opinion towards calling it routeContext? My rationale is that what we are kinda trying to expose through this new property is the context itself about the route, what's the name of the handler, and all the options used to set up the route among some other important configuration in the context of the route defined.

I think "Info" conveys the same thing, but I don't have a super strong opinion about it. I'll say that "Context" will likely require more explaining about why it is "Context", whereas "Info" is likely self explanatory.

[climba03003]

If we plan to refactor the option inside request.info, we better conclude the action faster.
It would be a pain if the process wait for too long.

[jsumners]

If we plan to refactor the option inside request.info, we better conclude the action faster. It would be a pain if the process wait for too long.

Yes. So @fastify/core should really speak up if they think a new deprecation cycle would be too disruptive.

[Eomm]

Renaming request.routeOptions

Renaming request.routeOptions to something else could be one of the Fastify v5 changes. I think request.info is too generic due request.log.info.

I think the scope of the property is to get access to the route's option that the user sets when the route is registered.

Review the properties

I think we could reduce the confusion by:

• request.routerPath should be deprecated for request.routeOptions.url
• request.routerMethod should be deprecated for request.routeOptions.method
• request.routeConfig should be deprecated for request.routeOptions.config
• request.routeSchema should be deprecated for request.routeOptions.schema
• Add request.routeOptions.handler

[Uzlopak]

what speaks again request.route ?

[jsumners]

Renaming request.routeOptions

Renaming request.routeOptions to something else could be one of the Fastify v5 changes. I think request.info is too generic due request.log.info.

I think the scope of the property is to get access to the route's option that the user sets when the route is registered.

Review the properties

I think we could reduce the confusion by:

• request.routerPath should be deprecated for request.routeOptions.url
• request.routerMethod should be deprecated for request.routeOptions.method
• request.routeConfig should be deprecated for request.routeOptions.config
• request.routeSchema should be deprecated for request.routeOptions.schema
• Add request.routeOptions.handler

In this case I would be much more in favor of the routeContext name.

[zekth]

I think routeContext is better as we are documenting context as runtime context in any part of the framework. It would have been confusing if we had request-context for example. Might just need rewording in the doc. However i'm +1.

[metcoder95]

SGTM 👍

[jsumners]

It sounds like we are okay with:

1. Deprecating the newly added req.routeConfig property
2. Adding a new req.routeContext property that looks like:

{
	url,
	method,
	config, // looks like the current req.routeConfig
	schema,
	handler // reference to the actual function which would include `handler.name` for named functions
}

[Eomm]

(As suggestion) I would proceed by steps here to ease the review/merge/release:

• enhance the routeOptions by adding handler, schema and config
• deprecate routeOptions and rename it to routeContext
• deprecate routerPath, routerMethod, routeConfig, routeSchema

[cesarvspr]

Seems like schema and handler are already present on the routeOptions, which makes missing only config @Eomm

[Eomm]

Huston, we have a problem - the docs need to be updated then 😄

from https://www.fastify.io/docs/latest/Reference/Request/#request

[cesarvspr]

https://www.fastify.io/docs/latest/Reference/Request/#request

here we can see schema but not handler -> https://www.fastify.io/docs/latest/Reference/Hooks/#onroute

this is about the same subject right? routeOptions @Eomm

[Eomm]

this is about the same subject right?

I don't think so, request.routeOptions (that will become request.routeContext) is not the same object that the onRoute hook gets as the argument - for sure the name is confusing.. but we will address this issue while fixing the docs

This issue refers to the request's object

[cesarvspr]

I don't think so

Sorry about that, my mistake

If so, I think your statement stays true from what I was able to debug