Mount: Remove unnecessary tracing #415
Conversation
| // This code is shared between GVFS.exe and GVFS.Service.exe | ||
| // GVFS.exe failures will be retried by GVFS.Service.exe | ||
| // Warn the user and allow the caller to determine how to handle the error | ||
| tracer.RelatedWarning(errorMessage); |
There was a problem hiding this comment.
Though more to the point, I think we can just drop this line altogether. The callers already log all errors, so this is just duplicating the work. I think that applies to all the tracer calls in this method - I think it's not really this method's business to decide what is an error and what is not.
There was a problem hiding this comment.
@sanoursa makes sense thanks! I was debating dropping the line, but thinking if we do it for one we'd want to drop them all. Definitely cleaner this way.
| @@ -373,7 +370,7 @@ public bool IsReady(JsonTracer tracer, string enlistmentRoot, out string error) | |||
| return | |||
| IsServiceRunning(tracer) && | |||
| IsNativeLibInstalled(tracer, new PhysicalFileSystem()) && | |||
| TryAttach(tracer, enlistmentRoot, out error); | |||
| TryAttach(enlistmentRoot, out error); | |||
There was a problem hiding this comment.
It looks like the error being returned is not being logged by the MountVerb:
if (!GVFSPlatform.Instance.KernelDriver.IsReady(tracer, enlistment.EnlistmentRoot, out errorMessage))
{
tracer.RelatedInfo($"{nameof(MountVerb)}.{nameof(this.Execute)}: Enabling and attaching ProjFS through service");
if (!this.ShowStatusWhileRunning(
() => { return this.TryEnableAndAttachPrjFltThroughService(enlistment.EnlistmentRoot, out errorMessage); },
$"Attaching ProjFS to volume"))
{
Could you add this error message here:
tracer.RelatedInfo($"{nameof(MountVerb)}.{nameof(this.Execute)}: Enabling and attaching ProjFS through service");
So that we can see in the mount verb logs why GVFS decided to ask the service?
There was a problem hiding this comment.
@wilbaker Let me know what you think of the log message. That sort of thing is not my forte :-).
There was a problem hiding this comment.
I think the message sounds good, but I think we can combine these into a single call and use EventMetadata. I'll add a comment by the new code with what I mean.
GVFS/GVFS/CommandLine/MountVerb.cs
Outdated
| @@ -117,6 +117,7 @@ protected override void Execute(GVFSEnlistment enlistment) | |||
|
|
|||
| if (!GVFSPlatform.Instance.KernelDriver.IsReady(tracer, enlistment.EnlistmentRoot, out errorMessage)) | |||
| { | |||
| tracer.RelatedInfo("ProjFs failed to start due to " + errorMessage); | |||
| tracer.RelatedInfo($"{nameof(MountVerb)}.{nameof(this.Execute)}: Enabling and attaching ProjFS through service"); | |||
There was a problem hiding this comment.
You could do a single tracer call here like this:
tracer.RelatedEvent(
EventLevel.Informational,
$"{nameof(MountVerb)}_{nameof(this.Execute)}_EnablingKernelDriverViaService",
new EventMetadata
{
{ "KernelDriver.IsReady_Error", errorMessage }
});
There was a problem hiding this comment.
And if you want to include a message you can just add it to the EventMetadata:
{ TracingConstants.MessageKey.InfoMessage, "Your message here" }
There was a problem hiding this comment.
I added a message for clarity, so the line reads:
[2018-10-25 11:06:47 -04:00] MountVerb_Execute_EnablingKernelDriverViaService {"KernelDriver.IsReady_Error":"Attaching the filter driver resulted in: 2147942405","Message":"Service will retry"}
Allow the caller to determine how to handle the error. This is because a command line error may fail due to permissions and be retried successfully by the service.
jrbriggs
added
the
affects: live-site
We may fail to attach the filter from GVFS.exe due to the user not running as admin. In this case we will retry the mount via GVFS.Service.exe. We don't want GVFS.exe to log a failure telemetry event, since it may get successfully retried. This change removes the tracing, since the if called via the service we will still log an error here and send it back to GVFS.exe.
If we want to get more precise the error code I see on failing to attach the filter without permissions is (2147942405 or 0x80070005). We could add specific handling for that code if we think that's more appropriate.