Skip to content

feat: log export blacklist#1881

Merged
wusteven815 merged 9 commits intodeephaven:mainfrom
wusteven815:export-log-blacklist
Apr 9, 2024
Merged

feat: log export blacklist#1881
wusteven815 merged 9 commits intodeephaven:mainfrom
wusteven815:export-log-blacklist

Conversation

@wusteven815
Copy link
Copy Markdown
Contributor

@wusteven815 wusteven815 commented Mar 19, 2024

  • Adds Stop exporting client from support logs #1245
    • Add a replacer function to all JSON.stringify to filter out blacklisted path
    • Add the following paths to blacklist: api, client, dashboardData.defaultLayout.connection, layoutStorage, storage

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 19, 2024

Codecov Report

Attention: Patch coverage is 5.00000% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 46.22%. Comparing base (65db969) to head (3647daa).
Report is 24 commits behind head on main.

Files Patch % Lines
packages/code-studio/src/log/LogExport.ts 5.00% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1881      +/-   ##
==========================================
+ Coverage   46.14%   46.22%   +0.07%     
==========================================
  Files         636      647      +11     
  Lines       38072    38283     +211     
  Branches     9628     9687      +59     
==========================================
+ Hits        17570    17698     +128     
- Misses      20449    20532      +83     
  Partials       53       53              
Flag Coverage Δ
unit 46.22% <5.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wusteven815 wusteven815 marked this pull request as ready for review March 19, 2024 18:52
@wusteven815 wusteven815 self-assigned this Mar 19, 2024
@wusteven815
Copy link
Copy Markdown
Contributor Author

Do we still want to blacklist the dashboardData.defaultLayout.connection and layoutStorage.storageService.connection paths?

@wusteven815 wusteven815 linked an issue Mar 21, 2024 that may be closed by this pull request
@mofojed
Copy link
Copy Markdown
Member

mofojed commented Mar 22, 2024

@wusteven815 yes we should blacklist connection as well.

@mattrunyon
Copy link
Copy Markdown
Collaborator

We should blacklist these top level keys as well as they're JS API items and don't seem to add anything useful from what I can tell

  • api
  • storage
  • layoutStorage

I think client is stored in redux in enterprise only as I don't see it in the exported logs on latest

Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

We should be using JSON pointers. Also allow passing in a blacklist for Enterprise to use.

Here's an example exported from Enterprise for your testing benefit:
redux.json

client is at the root, so no need to blacklist everything.

Comment thread packages/code-studio/src/log/LogExport.ts Outdated
Comment thread packages/code-studio/src/log/LogExport.ts Outdated
Comment thread packages/code-studio/src/log/LogExport.ts Outdated
@wusteven815 wusteven815 requested a review from mofojed April 2, 2024 17:52
Comment thread packages/code-studio/src/log/LogExport.ts Outdated
Comment thread packages/code-studio/src/log/LogExport.ts Outdated
@wusteven815 wusteven815 requested a review from mofojed April 3, 2024 21:15
Comment thread packages/code-studio/src/log/LogExport.ts Outdated
Comment thread packages/code-studio/src/log/LogExport.ts
@wusteven815
Copy link
Copy Markdown
Contributor Author

I noticed that dashboardData.defaultLayout.connection and layoutStorage.storageService.connection no longer exist. Could dashboardData.defaultLayout.connection have been renamed to dashboardData.default.connection?

@wusteven815 wusteven815 requested a review from mofojed April 4, 2024 17:16
@mofojed
Copy link
Copy Markdown
Member

mofojed commented Apr 5, 2024

@wusteven815 yes it is dashboardData.default.connection. Also dashboardData.default.sessionWrapper.dh should be blacklisted as well.

mofojed
mofojed previously approved these changes Apr 5, 2024
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Looks good. Should blacklist dashboardData.default.connection and dashboardData.default.sessionWrapper.dh as well.

Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

We should move this into one of the util packages so DHE can use it with its own blacklist, but that can be a separate PR.

@wusteven815 wusteven815 merged commit d3fb28a into deephaven:main Apr 9, 2024
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 9, 2024
@wusteven815 wusteven815 deleted the export-log-blacklist branch April 11, 2024 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop exporting client from support logs

3 participants