Skip to content

Log digest file name when updating trust #12812

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

Merged
Joyce Er (joyceerhl) merged 2 commits into from
Jul 13, 2020
Merged

Log digest file name when updating trust #12812

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8e8b62
Log digest file name
joyceerhl Jul 8, 2020
2c3ea52
Only log each notebook file once
joyceerhl Jul 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/client/datascience/interactive-ipynb/digestStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify';
import * as os from 'os';
import * as path from 'path';
import { Uri } from 'vscode';
import { traceError } from '../../common/logger';
import { traceError, traceInfo } from '../../common/logger';
import { isFileNotFoundError } from '../../common/platform/errors';
import { IFileSystem } from '../../common/platform/types';
import { IExtensionContext } from '../../common/types';
Expand All @@ -27,6 +27,7 @@ export class DigestStorage implements IDigestStorage {
const fileLocation = await this.getFileLocation(uri);
// Since the signature is a hex digest, the character 'z' is being used to delimit the start and end of a single digest
await this.fs.appendFile(fileLocation, `z${signature}z\n`);
traceInfo(`Wrote trust for ${uri.toString()} to ${fileLocation}`);

@DonJayamanne Don Jayamanne (DonJayamanne) Jul 8, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd add a property to track whether we've logged this or not, else we could end up with a lot of unnecessary messages being logged, when we only need the first such log entry.

e.g.

if not loggedd earlier:
	traceInfo...

@joyceerhl Joyce Er (joyceerhl) Jul 8, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Then we'd need to remember which files we've seen before, since multiple notebook files could be edited simultaneously and we'd want to log each ipynb's corresponding digest file location at least once. Does your review still apply?

@IanMatthewHuff Ian Huff (IanMatthewHuff) Jul 8, 2020

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.

I think that we would have to do this logging check per file then. Don is right in that this would be so noisy with autosave on that it might not be a useful message if it logs every save.

@rchiodo Rich Chiodo (rchiodo) Jul 8, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think I would just debounce it. Logging every save isn't that noisy. It's once a second.

@DonJayamanne Don Jayamanne (DonJayamanne) Jul 8, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debouncing won't work as its possible we save a new file and that can get swallowed.

@DonJayamanne Don Jayamanne (DonJayamanne) Jul 9, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Debounce will not work, as you loose updates from other files.

@DonJayamanne Don Jayamanne (DonJayamanne) Jul 9, 2020 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Personally i don't see the point of logging every update, the assumption is that something will go wrong and we will end up looking at the log and the file to look for a specific hash.
I find that to be an unlikely scenario.
Nope, re-read richs comments, it about tracking the last update to the file.

@DonJayamanne Don Jayamanne (DonJayamanne) Jul 9, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure we need that either, anyways.

@joyceerhl Joyce Er (joyceerhl) Jul 10, 2020

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Jim Griesmer (@greazer) suggested, and I agree, that we should log this info at least once for users. It seems plausible that something might go wrong and we won't have a way of recovering that info (short of them giving us the URI to their file and us hashing it to figure out which digest file is relevant).

This info dominates the log, so we shouldn't log on each save either. And as Don mentioned, debouncing won't work. Therefore, anyone have any strong objections to me implementing this? #12812 (comment)

@rchiodo Rich Chiodo (rchiodo) Jul 10, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sure sounds fine.

}

public async containsDigest(uri: Uri, signature: string) {
Expand Down