immobiliare / fastify-metrics Goto Github PK

Add tests to check the working of TCP

The deprecation says we cannot access context from the request anymore in version 5

The configuration of the route should have fastifyPrefix and routesPrefix optional, but we have set it mandatory.

interface FastifyContextConfig {
        metrics: {
            /** The id for this route that will be used for the label */
            routeId: string;
            /** Normalized fastify prefix for this route */
            fastifyPrefix: string;
            /** Normalized prefix of the routes */
            routesPrefix: string;
        };
    }

I think it's useful to cite this section of the fastify documentation because it directly impacts us. We have specified in the README that we have to use await when registering, now we can also say why.

A 404 generates an internal server error because the plugin tries to send a metric using the reply decorator, which in turn checks for the presence of a routeId inside the metrics object. This object is not setup for a route that doesn't exists because we use the onRoute hook to do it. We should take this into account in the onError hook.

I think it could be useful to allow to pass a custom doc instance as discussed also for the StatsD client.

What channel should we use for the pre-releases? Currently we are just specifying a next channel.

The context is referring to the fastify instance used to register the plugin and not the instance that declared the route.

Some checks are flaky because they rely on counting the ms passed. Let's try to use fake timers in those cases, otherwise it's better to remove them.

We should handle this deprecation

I think it is possible to check that a route id is duplicated before the server starts. Maybe it could be useful.

This option would allow to customise the health metrics that we want to send.

It's cleaner and clearer to use a boolean to mark a route as ignored. It also could be useful to allow the usage of the rootId for some custom logics.

I think it could be nice to see if it's possible to integrate this library with other metric libraries (even not statsd ones) using our client interface. I am not sure if it's possible but I think it's worth a try and, in case it works, add some examples in the docs.

I think it could be useful to allow overriding the fixed paths documented in https://github.com/immobiliare/fastify-metrics#metrics-collected

We should call and await the connect method of Dats to allow the working of TCP.

In light of what has been reported in immobiliare/fastify-sentry#615 I think we should align the types definitions to what we did in that plugin because it follows how the other types definitions in the official fastify plugins are implemented.

Having the tests in a single file has become too cumbersome to handle. We could use a refactor.

We could refactor the plugin entry point moving all generic functions and utilities in separate files. We might improve readability.

/assign @dnlup

I think it might be useful to add a response size metric in the set of stats collected. WDYT?

let metricsClient: MetricsPluginOptions['client']; // Option | CustomClient | undefined
metricsClient?.host 

leads to type error and i cannot cast witch type of client to use in my code

For some very particular use cases it can be useful to have the function that builds the metrics label string exposed(in dynamic mode) and have it accept a custom routeId to override the route metrics label.

Retrieving routeId from request.context leads to type error due to the missing definition of metrics object.

something like:

declare module 'fastify' {
    interface FastifyContextConfig {
        metrics: {
             routeId: string
        }
    }
}

Evaluate the possibility of throwing in dynamic mode when getLabel function is not passed

It might be useful to get a basic view on the performance impact of the PRs.

We can add Snyk.

What do you think of trackAsyncCbTime function to trace async function resolve's time?

async function asyncFunction(a, b, c) {
    await doSomething(a, b, c);
    return doSomething2(a, b, c);
}

const asyncFunctionOutput = await fastify.trackAsyncCbTime(
    { scope: 'something', label: 'test' },
    asyncFunction,
    a,
    b,
    c
);

WDYT?

Is this something that could be useful? We could filter by routeId.

Hi! First, wanna say great work for implementing automated metrics in fastify.

Currently we're trying to use fastify-metrics version 9.x with nestjs. Due to some limitation we cannot upgrade nestjs to latest version. When exploring our usecase, we cannot add custom labels with existing implementation while we can do it in version 10.x or above.

Is it possible to backport customLabels functionality in version 9.x? Wouldn't mind to raise a PR here.

If you pass a configuration like this:

fastify.register(plugin, {
    client: {
        host: 'udp://127.0.0.1:4000',
        namespace: 'test',
    },
    routes: {
        timing: false,
        // TODO: add responseSize
        /**
         * Route hit counter
         */
        hits: false,
        /**
         * Route errors counter
         */
        errors: false,
    },
    health: false,
});

You can use the method sendCounterMetric inside the reply, but the label is undefined.

To fix it we can remove the following check:

        if (
            config.routes.timing ||
            config.routes.errors ||
            config.routes.hits
        ) {

Things to look for:

• examples still referencing old config object in route setup (routeId)
• specificy context.config is on both reply and request
• specify getLabel should not have a . at the beginning and the end of the string.
• specify that getLabel is needed in dynamic mode
• add a migration guide

Scope requests/respose flags inside a dedicated key. Use the health key to pass custom sampler options too.

The current types do the overloading of Fastify default types, this creates the problem that we found the plugin types also in data type not reached from the plugin.

fastify.get<ReqReplyMetrics, MetricsConfig>(
        '/',
        { config: { metrics: { metricId: '' } } },
        (request, reply) => {}
    );

For this reason, we can evaluate the migration to generics typing, in this way, it is the user's responsibility to type his data.