Skip to content

fix(application-generic): logging levels #8111

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

Merged

Conversation

djabarovgeorge
Copy link
Contributor

What changed? Why was the change needed?

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Apr 10, 2025

Copy link

netlify bot commented Apr 10, 2025

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 2a4e273
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/67fe5bc3ffa35c0008d2e324
😎 Deploy Preview https://deploy-preview-8111.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@djabarovgeorge djabarovgeorge marked this pull request as ready for review April 14, 2025 10:22
@djabarovgeorge djabarovgeorge changed the title wip fix(application-generic): logging levels fix(application-generic): logging levels Apr 14, 2025

return {
exclude: [{ path: '*/health-check', method: RequestMethod.GET }],
assignResponse: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

customProps is not needed because of this new flag that was introduced in pino 4.2 version.

customProps: (req: any, res: any) => ({
        user: {
          userId: req?.user?._id || null,
          environmentId: req?.user?.environmentId || null,
          organizationId: req?.user?.organizationId || null,
        },
        authScheme: req?.authScheme,
        rateLimitPolicy: res?.rateLimitPolicy,
      }),

pinoHttp: {
customLevels: loggingLevelSet,
level: values.level,
useOnlyCustomLevels: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the end i kept the custom levels, this flag is making sure that they are not overridden by the built-in levels in pino

Comment on lines +15 to +21
trace: 10,
debug: 20,
info: 30,
verbose: 20,
debug: 10,
warn: 40,
error: 50,
fatal: 60,
none: 70,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alignment with the pino logger levels with additional 'none' in case we want to silence the logs.

Comment on lines +28 to +29
if (process.env.LOGGING_LEVEL || process.env.LOG_LEVEL) {
logLevel = process.env.LOGGING_LEVEL || process.env.LOG_LEVEL;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

backward compatibility for the old LOGGING_LEVEL environment variable.

@@ -159,7 +159,7 @@ jobs:
- name: Run Lint, Build, Test
uses: mansagroup/nrwl-nx-action@v3
env:
LOGGING_LEVEL: 'info'
LOG_LEVEL: 'info'
Copy link
Contributor

Choose a reason for hiding this comment

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

@merrcury do we need to makes some updates on TF deployment aswell?

Copy link
Member

Choose a reason for hiding this comment

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

No, We need to make this change just in secret Manager

redact: {
paths: redactFields,
censor: customRedaction,
Copy link
Contributor

Choose a reason for hiding this comment

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

What this one was doing originally?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, I see it's a blank function

@djabarovgeorge djabarovgeorge merged commit ce85343 into next Apr 16, 2025
29 of 30 checks passed
@djabarovgeorge djabarovgeorge deleted the nv-5651-investigate-why-log-levels-dont-work-in-production branch April 16, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants