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(core): add grafana observability to logto #136

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

bozzelliandrea
Copy link

Ticket:

Description

With this PR we add grafana instrumentation for observability integration. The application will collect traces and metrics, for the logs, we use the manual publishing logger configuration

Type

  • Dependency upgrade
  • Bug fix
  • New feature
  • Dev change

Checklist:

  • I have added tests that prove my fix is effective or that my feature works

Screenshots:

N/A

@bozzelliandrea bozzelliandrea marked this pull request as ready for review September 17, 2024 13:53
docker-compose-db.yml Outdated Show resolved Hide resolved
packages/core/src/instrumentation.ts Outdated Show resolved Hide resolved
@alfonsograziano
Copy link
Collaborator

Hey @bozzelliandrea that looks great! Might I ask you just one change? At the moment, in order to track all the changes that we're doing (we don't want to diverge too much from the original repo), we're including a OGCIO comment on every change we make. If you search for OGCIO in the codebase you can find multiple examples. Can I please ask you to stick with the same pattern? That helps us a lot during the versions update :)

@bozzelliandrea
Copy link
Author

Hi @alfonsograziano , thanks for the feedback, I added some comments for the lines introduced with the feature.

Copy link
Collaborator

@alfonsograziano alfonsograziano left a comment

Choose a reason for hiding this comment

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

LGTM - just on a side note, next week we will have a pen test on Logto and we would like to NOT merge PRs (if not critical).

Would it be fine for you to keep this one opened and ready to be merged as soon as we complete the pen test?

It will be on the Staging env (so Dev shouldn't be affected), but still would be ideal to not introduce any change in the code for the moment being

@bozzelliandrea
Copy link
Author

Sure no problem, @alfonsograziano we merge after pen-test.

@andreacappadona17
Copy link

Setting this PR to draft to avoid accidental merges.

@andreacappadona17 andreacappadona17 marked this pull request as draft September 20, 2024 09:29
@andreacappadona17
Copy link

@bozzelliandrea
Let's wait for the o11y sdk package to be ready for release before reworking this PR to avoid having to heavily refactor this later on.

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.

4 participants