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.

Sign up for GitHub

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

Jump to bottom

hook scrubber as logrus formatter #17781

Merged
merged 1 commit into from
Jun 29, 2023
Merged

hook scrubber as logrus formatter #17781

merged 1 commit into from
Jun 29, 2023

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented May 30, 2023 •
edited
Loading

Description

We reconsidered approach and decided to redact all nested properties if they don't implement Scrubbable interface.

Previous attempt I tried first to scrub before formatting and wasted too much time tuning scrubbing itself (although did fix some cases). But it turned out that scrubbing mutates data as for now, so we have to make the deep copy otherwise it breaks for instance startup of image build and public api. So I went for JSON formatting first and then scrubbing of JSON. It implies though additional unmarshalling/marshalling. It makes logging 2-3x times slower. I am not sure we should invest more time in tuning since benchmarking still shows that on average for complex data structures it takes 10000 ns.

I also explored using logurs hooks, but they don't have impact on the control flow, see [1]

Related Issue(s)

fix SID-285

How to test

  • Start a workspace.
  • Inspect different go component logs for some noisy errors when scrubbing prevents logging.
  • Check public api logs for instance for redact.

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

/hold

@akosyakov
Copy link
Member Author

@csweichel Is it fine to replace https://github.com/gitpod-io/gitpod/blob/main/components/common-go/log/redact.go with scrubbing or better in another PR?

@akosyakov akosyakov marked this pull request as ready for review June 1, 2023 09:35
@akosyakov akosyakov requested review from a team June 1, 2023 09:35
@akosyakov akosyakov requested a review from a team as a code owner June 1, 2023 09:35
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Jun 1, 2023
@akosyakov
Copy link
Member Author

I moved general improvements to scrubber into #17934 to unblock https://github.com/gitpod-io/gitpod-dedicated/pull/1332

@roboquat roboquat added size/XL and removed size/XXL labels Jun 14, 2023
@roboquat roboquat added size/XXL and removed size/XL labels Jun 14, 2023
iQQBot
iQQBot approved these changes Jun 14, 2023
View reviewed changes
components/common-go/log/log.go Outdated Show resolved Hide resolved
@akosyakov akosyakov force-pushed the ak/logrus_scrub branch 3 times, most recently from 4da0878 to 10c6745 Compare June 22, 2023 07:36
@akosyakov akosyakov force-pushed the ak/logrus_scrub branch 2 times, most recently from 54a3dd2 to 3e6cb58 Compare June 23, 2023 08:02
@akosyakov
Copy link
Member Author

akosyakov commented Jun 23, 2023 •
edited by github-actions bot
Loading

/gh run recreate-vm=true

Comment triggered a workflow run

Started workflow run: 5354466990

  • recreate_vm: true

@akosyakov
Copy link
Member Author

/unhold

mads-hartmann
mads-hartmann approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@mads-hartmann mads-hartmann left a comment

Choose a reason for hiding this comment

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

Awesome work iterating on this and getting it through.

@roboquat roboquat merged commit f580e6b into main Jun 29, 2023
@roboquat roboquat deleted the ak/logrus_scrub branch June 29, 2023 09:16
@roboquat roboquat added deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production labels Jun 29, 2023
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csweichel csweichel csweichel left review comments

@iQQBot iQQBot iQQBot approved these changes

@mads-hartmann mads-hartmann mads-hartmann approved these changes

@easyCZ easyCZ easyCZ approved these changes

@WVerlaek WVerlaek WVerlaek approved these changes

Assignees
No one assigned
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production size/XXL team: IDE team: security-infrastructure-and-delivery team: staff-engineers team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@akosyakov @easyCZ @mads-hartmann @csweichel @iQQBot @WVerlaek @roboquat