feat(server): added prometheus metrics to messaging server #546
Conversation
There was a problem hiding this comment.
Looks good 👍
Be careful to not break design principles though. The express object isn't given to services anywhere because it's not the services job to interact with the API.
Generally in this codebase :
- Api : makes something queryable
- Stream : sends data to some place
- Service : any kind of processing of data from the DB or from events
- Api and Stream never interact with each other
- Api and Stream depend on Service, never other way around
The express object is made available in the constructor of the base Api object (it's the root parameter). You can take that to extract the code in you service that depends on having the express router and make a new MetricsApi class. If it's not possible to separate the two cleanly then it's preferable to put everything in the API layer regardless.
| this.logger.debug(`${clc.blackBright(`[${id}]`)} ${clc.cyan('metrics')} messages incremented`) | ||
| messagesCount.inc() | ||
| } | ||
|
|
||
| private async handleConversationCreated({ conversation: { id } }: ConversationCreatedEvent) { | ||
| this.logger.debug(`${clc.blackBright(`[${id}]`)} ${clc.cyan('metrics')} conversations incremented`) | ||
| conversationsCount.inc() |
There was a problem hiding this comment.
This might impact performance. If would put this before :
if (yn(process.env.LOGGING_ENABLED)) {
The debugger in messaging is very dumb, doing logger.debug doesn't stop it from printing messages even if logging is disabled
packages/server/src/index.ts
Outdated
| import { Entry, start } from '@botpress/messaging-framework' | ||
| import './rewire' | ||
| import { Api } from './api' |
There was a problem hiding this comment.
Not putting import './rewire' at the top is likely going to break import from @botpress/messaging-framework when using yarn dev
| import * as promex from '@bpinternal/promex' | ||
| import { defaultNormalizers } from '@promster/express' | ||
| import clc from 'cli-color' | ||
| import type { Express } from 'express' |
There was a problem hiding this comment.
Never seen import type before
There was a problem hiding this comment.
This prevents the package from being bundled since it's only a typescript concept. import type gives you only the access to the types of a package.
packages/server/src/metrics/index.ts
Outdated
| @@ -0,0 +1 @@ | |||
| export * from './metrics' | |||
There was a problem hiding this comment.
The "re-export from index.ts" pattern is not used in the codebase so it's a bit inconsistent to use it here
| import type { Express } from 'express' | ||
| import { App } from './app' | ||
|
|
||
| export class Interceptor { | ||
| constructor(private app: App, private express: Express) {} | ||
|
|
||
| async setup() { | ||
| this.app.metrics.init(this.express) | ||
| } | ||
| } |
There was a problem hiding this comment.
Not really what I meant but whatever. I think it's time I unsubscribe from this repo lol I'll stop bothering you with nitpick comments
This adds prometheus metrics to the messaging server. It exposes an endpoint on port 9090 for Prometheus.
There are 2 custom metrics added
messages_totalandconversations_totalfor the total of messages and conversations created by the currentmessagingserver.