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

feat(apm): added apm configuration setup #167

Merged
merged 3 commits into from
Sep 3, 2021
Merged

feat(apm): added apm configuration setup #167

merged 3 commits into from
Sep 3, 2021

Conversation

michaelmass
Copy link
Contributor

@michaelmass michaelmass commented Sep 2, 2021

This adds the Sentry sdk inside the messaging server to monitor application metrics (apm).

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2021

CLA assistant check
All committers have signed the CLA.

@laurentlp laurentlp self-requested a review September 2, 2021 15:18
@samuelmasse
Copy link
Contributor

What does this do? Can you add a description to you PR. There are prettier errors in the ordering of imports. Please make sure to install the prettier extension for vscode before committing to this repo. The name of the PR should also follow standard commit norms. So : chore(server): add apm

Comment on lines 55 to 64
const apmEnabled = yn(process.env.APM_ENABLED)

if (apmEnabled) {
Sentry.init({
integrations: [new Sentry.Integrations.Http({})]
})

this.root.use(Sentry.Handlers.requestHandler())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extract that in a setupApm function. For the bottom bit, you can extract that in a setupApmErrorHandler function. We want to avoid having anything complicated happen in this main setup function. We mostly just call other services's setup function from here and do some small single line stuff.

@@ -9,6 +9,8 @@ import { MessageApi } from './messages/api'
import { SocketManager } from './socket/manager'
import { SyncApi } from './sync/api'
import { UserApi } from './users/api'
import * as Sentry from '@sentry/node'
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure that ESLint and Prettier run on your code. This should be automatic when using VSCode.

@samuelmasse
Copy link
Contributor

@michaelmass can you fix these issues so we can merge this?

@michaelmass michaelmass changed the title added apm feat(apm): added apm configuration setup Sep 3, 2021
Copy link
Contributor

@laurentlp laurentlp left a comment

Choose a reason for hiding this comment

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

LGTM!

@samuelmasse samuelmasse merged commit a1cc6a0 into botpress:master Sep 3, 2021
@michaelmass michaelmass deleted the mm-apm branch September 7, 2021 12:41
@laurentlp laurentlp mentioned this pull request Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants