Skip to content

fix(#10357): prevent DEBUG logs from appearing in production#10376

Closed
AmirSaudagar55 wants to merge 16 commits intomedic:masterfrom
AmirSaudagar55:10357-fix-debug-logs
Closed

fix(#10357): prevent DEBUG logs from appearing in production#10376
AmirSaudagar55 wants to merge 16 commits intomedic:masterfrom
AmirSaudagar55:10357-fix-debug-logs

Conversation

@AmirSaudagar55
Copy link
Copy Markdown
Contributor

@AmirSaudagar55 AmirSaudagar55 commented Oct 13, 2025

Summary

Fixes #10357 where DEBUG logs appeared in production despite NODE_ENV=production.

Changes

  • Updated Winston logger to respect LOG_LEVEL environment variable.
  • Defaults to 'info' when NODE_ENV=production.
  • Verified logging behavior in both production and development environments.

Testing

  1. Production environment (NODE_ENV=production)

    • DEBUG logs are hidden.
    • Screenshot attached:
      production_env1
      production_env
  2. Development environment (NODE_ENV=development)

    • DEBUG logs are visible.
    • Screenshot attached:
      develpment_env

Notes

  • This does not change any functional behavior of the API.
  • Ensures logs comply with CHT default log level conventions.

Copy link
Copy Markdown
Member

@sugat009 sugat009 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Member

@1yuv 1yuv left a comment

Choose a reason for hiding this comment

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

This doesn't fix the underlying problem. If NODE_ENV is not set, which is the not set by default, the environment will be development and still log at debug level. And this will continue print at debug level.

One way would be to consider environment as production when env is not set. This is not a good practice and patchy fix. But there doesn't appear to be any other option until #10356 is fixed.

@sugat009
Copy link
Copy Markdown
Member

One way would be to consider environment as production when env is not set.

I'd advise against that approach.

@1yuv Quick question: what challenges are you encountering when trying to set NODE_ENV values on the servers?

@1yuv
Copy link
Copy Markdown
Member

1yuv commented Oct 15, 2025

@1yuv Quick question: what challenges are you encountering when trying to set NODE_ENV values on the servers?

This issue #10356 explains the challenges @sugat009 . FYI, I checked on many of of the production instances medic hosts and none of them have NODE_ENV set.

@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

@1yuv @sugat009 Here is another approach which can be consider :
Instead of forcing “production” when NODE_ENV is unset, make log level configurable directly via LOG_LEVEL and fallback safely to info.

  • If the user sets LOG_LEVEL, it overrides everything → full control.

  • If NODE_ENV=production → defaults to safe info.

  • If NODE_ENV is unset → still defaults to info (not risky assumption, just safety fallback).

Please let me know if this approach aligns with your expectations or if you’d suggest any refinements. 🙂

@sugat009
Copy link
Copy Markdown
Member

After re-reviewing the current code, I've identified an issue with the default logging level:

When the NODE_ENV environment variable is not explicitly set, our application defaults the environment to development (as seen in line 2). This implicitly sets the log level to debug.

Possible Solution

To ensure reliable and configurable logging across all environments, we can implement the following fix:

  1. Decouple Environment and Log Level: The application's core logic will be modified to default the log level to info (a more moderate setting) regardless of the NODE_ENV value. There could be cases when we need to debug the prod instances but not have the NODE_ENV be not-production.
  2. Use a Dedicated Variable: The log level will only be changed from the default if the LOG_LEVEL environment variable is explicitly set (e.g., LOG_LEVEL=debug or LOG_LEVEL=error).

This approach separates environment optimizations (NODE_ENV) from logging verbosity (LOG_LEVEL), giving us granular control and preventing accidental verbose logging in production.

#10356 should be addressed separately in another PR with another discussion.

Open for further discussion.

@AmirSaudagar55 AmirSaudagar55 requested a review from 1yuv October 21, 2025 13:53
@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

AmirSaudagar55 commented Oct 21, 2025

Should I Change the documentation too

@sugat009
Copy link
Copy Markdown
Member

Should I Change the documentation too

yes, that will be necessary, since the current documentation in medic/cht-docs#2030 seems to be referring to NODE_ENV for settings docs.

@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

Just updated the documentation.

@andrablaj
Copy link
Copy Markdown
Member

@sugat009 @1yuv is this PR ready to be merged?

@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

Hi, I noticed that the some tests are failing with a timeout error. Since this PR changes the default log level to info, those log lines which they are expecting, no longer appear.
I haven’t made any changes related to that area, so it might be a flaky CI issue.

@sugat009
Copy link
Copy Markdown
Member

@AmirSaudagar55 Thanks for flagging that out. I've rerun the failed jobs, let's see if they pass this time.

@sugat009
Copy link
Copy Markdown
Member

The CI workflows seem to have failed again. Looking into this.

@sugat009
Copy link
Copy Markdown
Member

@AmirSaudagar55 For the tests that are failing, the LOG_LEVEL is expected to be at debug level, which emits some extra logs which are in turn used to check the readiness of some services. Can you check those out? The tests are available in the root level package.json file.

Basically, the necessary change is to make sure that the LOG_LEVEL is debug when those tests are run.

@sugat009
Copy link
Copy Markdown
Member

@AmirSaudagar55 The CI seems to be failing. Can you check it out please?

@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

@sugat009 Yes working on it. I think setting log_level in commands mention in package.json is not effective, CI tests are using docker, so now i will try to add log level to debug in docker environement which is used by those tests specifically and will ensure that it should not be reflected in production environment.

@AmirSaudagar55
Copy link
Copy Markdown
Contributor Author

@sugat009 is it fine now?

@sugat009
Copy link
Copy Markdown
Member

sugat009 commented Dec 9, 2025

Thanks for the reminder, @AmirSaudagar55 . The previous builds seem to be failing. I've run the workflows which will run on the latest commit. Let's see.

@sugat009 sugat009 self-requested a review December 12, 2025 06:15
Copy link
Copy Markdown
Member

@sugat009 sugat009 left a comment

Choose a reason for hiding this comment

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

@AmirSaudagar55 thanks for your work on this.

@sugat009 sugat009 self-requested a review December 12, 2025 06:45
containers:
- env:
- name: LOG_LEVEL
value: '{{ .Values.sentinel.log_level | default "info" }}'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@AmirSaudagar55 the log_level: "info" needs to be added in the scripts/build/helm/values/base.yaml file as well. Beforehand your changes were passing because sentinel template was trying to access they key from the API service which did not exist and was defaulting to info but I thought the helm template had a typo and I changed the value in the sentinel helm template to use the value from the sentinel itself which is causing the CI to fail.

The changes would look something like this in the scripts/build/helm/values/base.yaml file.

...
# API Service configuration <- Line 7
api:
  service:
    type: ClusterIP # DEFAULT: Good default for internal services
  log_level: "info" # DEFAULT: Log level for API service

# Sentinel Service configuration
sentinel:
  log_level: "info" # DEFAULT: Log level for Sentinel service
...

@andrablaj
Copy link
Copy Markdown
Member

@AmirSaudagar55 are you planning to continue the work on this PR? Is there any support you need? Please reach out if you get stuck!

@sugat009
Copy link
Copy Markdown
Member

Closing. Requested changes done here

@sugat009 sugat009 closed this Jan 20, 2026
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.

debug level messages printed on production instances

4 participants