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

Configure PII Anonymization for logs #12492

Merged
merged 29 commits into from Jun 14, 2023

Conversation

Tawakalt
Copy link
Contributor

@Tawakalt Tawakalt commented Jun 9, 2023

Proposed changes:

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Tawakalt Tawakalt changed the title Ato 668 configure pii anonymization for logs Configure PII Anonymization for logs Jun 9, 2023
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch 2 times, most recently from befc166 to 802726b Compare June 9, 2023 12:19
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from 802726b to 4ab4ffd Compare June 9, 2023 12:27
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from 75d5c74 to 00c0a33 Compare June 9, 2023 13:41
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch 2 times, most recently from 2a03fdd to deea9e5 Compare June 12, 2023 08:31
@Tawakalt Tawakalt marked this pull request as ready for review June 12, 2023 10:06
@Tawakalt Tawakalt requested a review from a team as a code owner June 12, 2023 10:06
@Tawakalt Tawakalt requested review from ancalita, radovanZRasa and tmbo and removed request for a team June 12, 2023 10:06
@Tawakalt Tawakalt mentioned this pull request Jun 12, 2023
4 tasks
rasa/core/processor.py Outdated Show resolved Hide resolved
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from deea9e5 to f78e9e0 Compare June 12, 2023 10:55
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Great work 😎 ! Some tiny comments from me.

rasa/utils/log_utils.py Outdated Show resolved Hide resolved
rasa/core/processor.py Show resolved Hide resolved
@Tawakalt Tawakalt requested a review from ancalita June 12, 2023 13:37
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from 41162b1 to 197b69b Compare June 12, 2023 14:00
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from e2b20ae to 4ce3dcd Compare June 13, 2023 16:32
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Decided to take another look at all the logs to make sure they're not using event as kwarg and discovered a few places that should be updated 😄

rasa/shared/core/events.py Outdated Show resolved Hide resolved
rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/core/nlg/interpolator.py Show resolved Hide resolved
@tmbo tmbo dismissed their stale review June 13, 2023 17:18

Outdated

rasa/core/brokers/pika.py Outdated Show resolved Hide resolved
Copy link
Member

@tmbo tmbo left a comment

Choose a reason for hiding this comment

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

one thing to consider for the future: some of the log messages are actually intended for our users - for them it would be be better to have natural language text in addition to the event name but can prob be tackled in another PR

rasa/core/channels/rest.py Outdated Show resolved Hide resolved
rasa/core/http_interpreter.py Outdated Show resolved Hide resolved
rasa/core/http_interpreter.py Outdated Show resolved Hide resolved
rasa/core/policies/memoization.py Outdated Show resolved Hide resolved
rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/nlu/test.py Outdated Show resolved Hide resolved
@Tawakalt
Copy link
Contributor Author

one thing to consider for the future: some of the log messages are actually intended for our users - for them it would be be better to have natural language text in addition to the event name but can prob be tackled in another PR

@tmbo Can that be added as an extra key to structlog? say event_info?
Cos I had a lot of concerns removing them completely.

If that's all that's required, I'll prefer to make the changes in this PR.

@tmbo
Copy link
Member

tmbo commented Jun 13, 2023

Yes I think an additional attribute should work. We can then sublcass the ConsoleRenderer and wrap its call function, something along the lines of (untested):

class HumanConsoleRenderer(ConsoleRenderer):
    def __call__(
        self, logger: WrappedLogger, name: str, event_dict: EventDict
    ) -> str:
        if "event_info" in event_dict:
            event_key = event_dict["event"]
            event_dict["event"] = event_dict["event_info"]
            event_dict["event_key"] = event_key

        return super(ConsoleRenderer).__call__(logger, name, event_dict)

and then use that instead of the console renderer in the structlog config. this would use the info key as the text line for console output but keep the event key for structured logging in json, e.g. on a server

@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from 49f0ac4 to cf464a1 Compare June 14, 2023 13:01
Copy link
Contributor

@radovanZRasa radovanZRasa left a comment

Choose a reason for hiding this comment

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

LGTM, with a small nitpick.

rasa/core/processor.py Outdated Show resolved Hide resolved
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from cf464a1 to 26bd0e5 Compare June 14, 2023 13:14
rasa/nlu/test.py Outdated Show resolved Hide resolved
@Tawakalt Tawakalt force-pushed the ATO-668-configure-pii-anonymization-for-logs branch from 26bd0e5 to 8a559a9 Compare June 14, 2023 13:28
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

💥

@github-actions
Copy link
Contributor

🚀 A preview of the docs have been deployed at the following URL: https://12492--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@Tawakalt Tawakalt enabled auto-merge June 14, 2023 13:51
@Tawakalt Tawakalt merged commit 641700d into main Jun 14, 2023
106 checks passed
@Tawakalt Tawakalt deleted the ATO-668-configure-pii-anonymization-for-logs branch June 14, 2023 13:56
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