912 better method for making sure jsonschema logging does not dump pii into logs #949

shahdyousefak · 2025-01-28T12:50:17Z

Please ensure you've followed the checklist and provide all the required information before requesting a review.
If you do not have everything applicable to your PR, it will not be reviewed!
If you don't know what something is or if it applies to you, ask!

Don't delete below this line.

Required Information

I referenced the issue addressed in this PR. --> Currently, logging behavior is not properly differentiated between Unicorn and Pegasus. This leads to unnecessary and potentially sensitive logs appearing in production. The goal of this PR is to ensure that:
Unicorn logs can be more verbose and include detailed error messages for debugging, including what may constitute PII-related logging, particularly when errors are thrown.
Pegasus logs are minimal, avoiding unnecessary verbosity particularly when errors are thrown.
I described the changes made and how these address the issue. --> configured logging_utils.ts and logging_utils.py to handle PII logging separately for Unicorn and Pegasus.
I described how I tested these changes. --> verified that logs correctly identify whether the service is running in Unicorn (PII logging enabled) or Pegasus (PII logging disabled). toggled the

environment:
      - PII_LOGGING_ENABLED=true

to discern output for pegasus/unicorn. Next, need to address how it will be picked up if pii logging is enabled.

Coding/Commit Requirements

I followed applicable coding standards where appropriate (e.g., PEP8)
I have not committed any models or other large files.

New Component Checklist (mandatory for new microservices)

I added an entry to docker-compose.yml and build.yml.
I created A CI workflow under .github/workflows. --> I updated the workflows to copy the config directory, which contains the script for PII logging, to ensure it is available during the build process; before, config/logging_utils.ts wasn’t found, breaking the build. (Similar to copying schemas like run: cp -R ../../schemas src/)

    modified:   .github/workflows/autour-handler.yml
    modified:   .github/workflows/high-charts-handler.yml
    modified:   .github/workflows/motd.yml
    modified:   .github/workflows/osm-streets-handler.yml
    modified:   .github/workflows/photo-audio-handler.yml
    modified:   .github/workflows/photo-audio-haptics-handler.yml

I have created a README.md file that describes what the component does and what it depends on (other microservices, ML models, etc.).

OR

I have not added a new component in this PR.

jaydeepsingh25

Too many issues, I will try to summarize everthying here:

Please make sure your source branch is in sync with main branch. Currently it is reverting a lot of code in main branch (Microservices Queue changes, healthchecks, text-followup preprocessor and handler)
There should be no changes in build.yml and docker compose files (other than setting logging env variable)
No change in dockerfiles apart from copying config folder
For the preprocessors/handlers where are setting debug log level, need to make sure we are not removing it - need to confirm if it still prints debug statements if you set log level to PII

Please feel free to raise a new PR if that's easier for you - just make sure you don't change any other code which is not related to logging!

jaydeepsingh25 · 2025-02-22T07:30:03Z

build.yml

why are we removing text-followup preprocessor and handler?

jaydeepsingh25 · 2025-02-22T07:33:11Z

docker-compose.yml

same, these should not removed from docker compose

jaydeepsingh25 · 2025-02-22T07:46:21Z

test-docker-compose.yml

-  text-followup:
-    environment:
-      LOG_PII: "true"
+      - SERVER_NAME=unicorn.cim.mcgill.ca


shouldn't we set PII_LOGGING_ENABLED true here?

jaydeepsingh25 · 2025-02-22T07:47:43Z

services/tat/Dockerfile

 # Copy the build output from the previous stage to the Nginx HTML directory
 COPY --from=build /app/dist/editor/ /usr/share/nginx/html

-# Add a static health check file


Please keep health checks

jaydeepsingh25 · 2025-02-22T07:54:08Z

handlers/high-charts/Dockerfile

-COPY /handlers/high-charts/ /usr/src/app
-RUN npm ci
+
+COPY /handlers/high-charts/package*.json ./


this line is not required

jaydeepsingh25 · 2025-02-22T08:11:10Z

orchestrator/Dockerfile

 EXPOSE 8080

 USER node
-


please add back healthcheck

jaydeepsingh25 · 2025-02-22T08:12:16Z

orchestrator/src/server.ts

    if (data["preprocessors"] === undefined) {
        data["preprocessors"] = {};
    }
    let currentPriorityGroup: number | undefined = undefined;


Please add back this code

jaydeepsingh25 · 2025-02-22T08:16:41Z

preprocessors/grouping/grouping.py

+configure_logging()

 app = Flask(__name__)
-logging.basicConfig(level=logging.DEBUG)


again, just confirm if removing this will still log debug statements?

jaydeepsingh25 · 2025-02-22T08:20:02Z

preprocessors/nominatim/package.json

    "ajv": "^8.9.0",
    "express": "^4.19.2",
-    "uuid": "^8.3.2"
+    "node-fetch": "^2.7.0",


you are adding extra packages and they are not related to logging.
sure you still need them?

jaydeepsingh25 · 2025-02-22T08:27:38Z

services/espnet-tts-fr/Dockerfile

@@ -1,6 +1,6 @@
 FROM pytorch/pytorch:1.12.1-cuda11.3-cudnn8-runtime

-RUN apt-get update && apt-get install -y libsndfile1 espeak-ng build-essential curl && rm -rf /var/lib/apt/lists/*


add back healthchecks

shahdyousefak added 6 commits January 27, 2025 18:50

pii logging to grouping

0798ad2

cherry picking pii logging for grouping

8ed6a52

semseg pii

ea865b0

preprocessors with pii logging (1)

c3a5ee8

preprocessors with pii logging (2)

a106817

pii logging for preprocessors (3)

1c4a380

shahdyousefak linked an issue Jan 28, 2025 that may be closed by this pull request

Better method for making sure jsonschema logging does not dump PII into logs #912

Closed

4 tasks

shahdyousefak added 23 commits February 5, 2025 14:43

adding .py handlers + nominatim PII logging

56c5df9

modifying logging_utils.ts (still not working)

855ff86

ensure Winston correctly recognizes pii as a valid log level

98cb233

all typescript related MS incorporate PII logging

2ed63ed

resolving merge conflicts

a6a9b87

build failures

c509793

adding winston lib

ca57737

test

d90f99a

test

cbec4e5

test

6aebdab

logging_utils.ts import error

42a2ac7

logging_utils.ts import error

5f0837c

logging_utils.ts import error

392de7d

logging_utils.ts import error

ef4db3e

logging_utils.ts import error

8616394

logging_utils.ts import error

4244e05

modifying nominatim workflow yaml

24147f6

modifying nominatim workflow yaml to copy config

9f114b5

adding copy config to workflows, winston download

63c492b

adding debug to autour handler workflow

4c644c1

fixing typo in workflows

308e22c

fixing typo in workflows

bd2e5cc

adding npm install to high-charts

e428433

shahdyousefak requested a review from jaydeepsingh25 February 11, 2025 11:52

shahdyousefak assigned jaydeepsingh25 Feb 11, 2025

jaydeepsingh25 requested changes Feb 22, 2025

View reviewed changes

jaydeepsingh25 assigned shahdyousefak and unassigned jaydeepsingh25 Feb 22, 2025

shahdyousefak closed this Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

912 better method for making sure jsonschema logging does not dump pii into logs #949

912 better method for making sure jsonschema logging does not dump pii into logs #949

Uh oh!

shahdyousefak commented Jan 28, 2025 •

edited

Loading

Uh oh!

jaydeepsingh25 left a comment

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

jaydeepsingh25 Feb 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -1,6 +1,6 @@
		FROM pytorch/pytorch:1.12.1-cuda11.3-cudnn8-runtime

		RUN apt-get update && apt-get install -y libsndfile1 espeak-ng build-essential curl && rm -rf /var/lib/apt/lists/*

912 better method for making sure jsonschema logging does not dump pii into logs #949

912 better method for making sure jsonschema logging does not dump pii into logs #949

Uh oh!

Conversation

shahdyousefak commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required Information

Coding/Commit Requirements

New Component Checklist (mandatory for new microservices)

Uh oh!

jaydeepsingh25 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shahdyousefak commented Jan 28, 2025 •

edited

Loading