Tracing Updates #2009

BlythMeister · 2018-06-29T15:34:28Z

Remove tracing from Assembly file. Tracing seems to be used when calling out to external process, but this function does not do that, so seems silly
Add wrapper functions for tracing, can be used as follows:

let taskToDo value =
        Trace.log value
        true

    let taskToDo2() =
        Trace.log "value"
        true

    let taskToDo3 value =
        Trace.log value

    let taskToDo4() =
        Trace.log "value"

    let taskToDo5 value (trace:Fake.Core.Trace.ISafeDisposable) =
        Trace.log value
        trace.MarkSuccess()

    let taskToDo51 (trace:Fake.Core.Trace.ISafeDisposable) value =
        Trace.log value
        trace.MarkSuccess()

    let taskToDo52 (trace:Fake.Core.Trace.ISafeDisposable) value =
        Trace.log value
        trace.MarkFailed()

    let taskToDo6 (trace:Fake.Core.Trace.ISafeDisposable) =
        Trace.log "value"
        trace.MarkSuccess()

    let taskToDo61 (trace:Fake.Core.Trace.ISafeDisposable) =
        Trace.log "value"
        trace.MarkFailed()

    let taskToDo7() =
        raise(new System.Exception("Error"))

    if (Trace.traceTaskFunction "Test" "Test" (fun () -> taskToDo "MY TEST LOGGING")) then Trace.log "It worked"
    if (Trace.traceTaskFunction "Test" "Test" (fun () -> taskToDo2())) then Trace.log "It worked"
    Trace.traceTaskFunction "Test" "Test" (fun () -> taskToDo3 "TEST 3")
    Trace.traceTaskFunction "Test" "Test" (fun () -> taskToDo4())
    Trace.traceTaskFunctionDetailed "Test" "Test" (fun t -> taskToDo5 "TEST 3" t)
    Trace.traceTaskFunctionDetailed "Test" "Test" (fun t -> taskToDo51 t "TEST 3")
    Trace.traceTaskFunctionDetailed "Test" "Test" (fun t -> taskToDo52 t "TEST 3")
    Trace.traceTaskFunctionDetailed "Test" "Test" (fun t -> taskToDo6 t)
    Trace.traceTaskFunctionDetailed "Test" "Test" (fun t -> taskToDo61 t)
    Trace.traceTaskFunction "Test" "Test" (fun () -> taskToDo7())

The only places where tracing is typically done is when shelling out to other applications. AssemblyInfoFile is completely within FAKE so it seems weird to trace these tasks as standard. Is someone requires tracing, they can wrap the call to these functions themselves

matthid · 2018-06-29T16:43:11Z

Yes removing tracing from assemblyinfofile is one version. I noticed that they produce quite a lot of output.

However, in an ideal world we would be able to control this better instead of removing them (Actually you can by implementing a custom ITraceListener. For example we could decide to only "show" top-level tasks and not print others (or make that opt-in on verbose)

Regarding the Function suffix: Somehow I don't like it but don't really have a better suggestion (so we can take them as is)

What do you think?

BlythMeister · 2018-06-29T16:52:38Z

I removed because it was inconsistent...happy to leave it in.
I'm also happy to not have the function I can make this in my own scripts to suit my needs as it makes tracing around a function easier.

matthid · 2018-06-29T16:55:29Z

Both changes are fine (and I agree we need changes here). I'm happy to take them as-is. Just trying to explain why those changes in particular have not been made yet. Maybe we should ask on twitter if people have better ideas then the Function suffix..

BlythMeister · 2018-06-29T16:57:41Z

Oh I get you now, sorry I misunderstood.

I'm not sure what you mean by function suffix (do you mean passing the function as a delegate)

matthid · 2018-06-29T17:23:35Z

@BlythMeister It's just about the name of the new functions...

BlythMeister · 2018-06-29T17:25:51Z

Oh right!
Yeah, I'm not attached to them in any way...im not that keen on them either, but couldn't think of anything better 😂

BlythMeister · 2018-06-30T17:54:18Z

@matthid how about simplifying to tracefn and tracefnAuto?

BlythMeister · 2018-07-02T08:41:57Z

@matthid i've updated the function names having the word task and function made no sense...i'm not sure what else to call them because essentially they are tracing a function :S

# Conflicts: # src/app/Fake.Core.Target/Target.fs # src/app/Fake.Core.Trace/TraceListener.fs

BlythMeister · 2018-07-02T11:20:12Z

🎉 it builds 😄

I have over 100 projects in a repo...this spamming 100 log lines isn't that useful. If this level is required, then you can trace around the calls to save the assembly info

matthid

Some hints regarding latest changes and another idea for the high-order function.

matthid · 2018-07-02T18:10:12Z

src/app/Fake.Core.Trace/TraceListener.fs

@@ -15,6 +17,8 @@ type KnownTags =
        match x with
        | Task n
        | Target n
+        | FinalTarget n
+        | FailureTarget n


This is a breaking change and you need to update the various listeners (AppVeyor, Travis, ...) to handle these new cases.
Problem is we don't have warning as errors on for "missing cases on match"

An alternative would be to add the info to the Target case and add a "Targettype" or something like that.

The only listener which mentions Target is the console one, so i didn't think this would be a breaking change...

Ok makes sense, so we go like this because adding another TargetType would most likely be more breaking.

matthid · 2018-07-02T18:12:10Z

src/app/Fake.Core.Target/Target.fs

-               let target = get name
-               runSimpleContextInternal target context) context
+                        let target = get name
+                        use t = Trace.traceFailureTarget target.Name (match target.Description with Some d -> d | _ -> "NoDescription") (dependencyString target)


instead of "NoDescription" we use null now and properly handle that case in the listeners.

A better solution would be to use string option (which now is at the table considering the more breaking addition - see other comment), see #1996

Updated and have also made this a string option and fixed all the listeners we have.

matthid · 2018-07-02T18:17:38Z

src/app/Fake.Core.Trace/Trace.fs

+/// Traces a function execution
+/// If no exception is thrown then trace is marked as success
+/// Any exception thrown will result in a mark failed and exception re-thrown 
+let traceFunction name description func =


What about an in-line high order function

useWith (traceTask name description) (fun _ -> // callback )

implemented like

let inline useWith (d:ISafeDisposable) f = use t = d try let result = f () in t.MarkSuccess(); result with _ -> t.MarkFailed(); reraise()

i like it, implemented :)

BlythMeister · 2018-07-02T19:17:41Z

@matthid updates made following your review :)

matthid · 2018-07-02T19:19:09Z

src/app/Fake.Core.Trace/Trace.fs

+/// Traces a function execution
+/// If no exception is thrown then trace is marked as success
+/// Any exception thrown will result in a mark failed and exception re-thrown 
+let inline useWith (d:ISafeDisposable) f =


yes it looks quite reasonable. I guess there are two versions, one where MarkSuccess is automatic and one where it is explicit.
I guess we could have either 2 functions or a boolean parameter. We could forward the t to f in order to allow f to change it's state internally.

(Reason is that calling nothing is equal to inconclusive)

Because the signature of f would be different it had to be 2 functions.

I wonder if we really need the manually set one or if we can just have the automatic one

matthid · 2018-07-02T19:23:35Z

Another design would be

Trace.traceTask name description |> Trace.useWith (fun _ ->
   // callback
)

instead of

Trace.useWith (Trace.traceTask name description) (fun _ ->
   // callback
)

with reversed parameters

BlythMeister · 2018-07-02T19:36:50Z

@matthid backing out the string option stuff....that is seriously breaking!

BlythMeister · 2018-07-02T19:50:51Z

This should be done (if it builds)

BlythMeister · 2018-07-02T20:02:07Z

Actually I like the pipe into Trace.useWith...seems cleaner.

I'll update to be like that tomorrow

matthid · 2018-07-02T20:19:36Z

@matthid backing out the string option stuff....that is seriously breaking!

@BlythMeister What was the error, is it on CI somewhere?

BlythMeister · 2018-07-02T20:25:54Z

There were over 100 places which needed updating to pass a string option rather than string..

matthid · 2018-07-02T20:36:30Z

Yes because the change bobbled to the top, which could be prevented. But I'm okish with using null as long as it is documented. We can later add another API traceTaskWithoutDesciption if we want. But I think this PR is already a good set of changes.

BlythMeister · 2018-07-02T20:47:59Z

Yeah I think separating so we sort that typing later would be good.
The only reason I didn't see the change in targets was because I didn't fetch!

I'll have another play with the trace wrapper function and see if I can get something which fits nicely.

BlythMeister · 2018-07-03T11:59:57Z

@matthid ok i think i've found a nice way to get this to work :)

Manual mode

Trace.traceTask "Name" "Description" 
|> Trace.useWith (Manual(fun t ->
   t.MarkSuccess()
   Console.WriteLine("Manual Set Success")
))

Automatic mode

Trace.traceTask "Name" "Description" 
|> Trace.useWith (Automatic(fun _ -> 
   Console.WriteLine("Automatic Set Success")
))

In both cases, if the func raises an exception, the trace is set to failed (even if you have called MarkSuccess()

I've also put the string option code in place and made it so Trace.traceTask still takes a string and not string option which is then checked for String.IsNullOrWhiteSpace before passing down.

matthid · 2018-07-03T12:35:39Z

I thought it would need to be

|> Trace.useWith (Trace.Manual(fun t ->

Because of RequireQualifiedAccess? Also the double set of braces ;)

Also I have no problem with providing the ISafeDisposable in the automatic mode because you might still want to call MarkFailed in your code on some condition (ie on a non-exception path).

BlythMeister · 2018-07-03T16:21:42Z

You know what...your right. I'll simplify tomorrow (England football tonight 😂)

…r function

BlythMeister · 2018-07-04T07:46:45Z

@matthid hopefully that's the last change :)

matthid · 2018-07-04T07:54:38Z

Sorry for the back and forth...

BlythMeister · 2018-07-04T07:57:08Z

No need to be sorry, i really enjoy adding stuff and the back and forth!

BlythMeister · 2018-07-04T14:40:17Z

@matthid if your happy with this change now, any ideas when we can get it in?
I'm having to work around it in my build scripts at the moment :)

matthid · 2018-07-04T14:52:45Z

Everything looks good, it will probably be part of the next 5.1.1 or 5.2 (more likely) release.
Before merging I just want to double check that no signatures of the public part of the API (Trace.* functions) changed behind the scenes. I'm OKish with changing the backend DU and existing listeners (want to double check that as well) as long as the consumer API is still compatible.

This is kind of hard to check in the github PR view.

BlythMeister · 2018-07-04T15:16:40Z

Is there a way to get a nuget package with this (and any other pre-release)?

matthid · 2018-07-04T15:22:34Z

If staging is fine I can merge it and look later. Problem with staging (currently) is that packages are deleted after two versions (basically two commits on release/next) this means you need to update often when I'm working on it and builds might break... (we got 1gb space on myget and that is basically just a couple of deployments, because we push so much stuff)

BlythMeister · 2018-07-04T15:53:03Z

That works for me...I'm currently re working our company build tools (hence so many prs) so I can live with fragile...what is the myget URL?

matthid · 2018-07-04T16:00:37Z

@BlythMeister Ok it is available after the build finished, the URLs are documented here: https://fake.build/contributing.html#Staging-environment

Staging is automatically deployed and always the latest state of the release/next branch (current PR: #2011). (Well the latest green vsts build to be exact). After the build finished it takes a couple of minutes to be available.

BlythMeister added 2 commits June 29, 2018 15:54

Add wrapper functions for tracing

6f196c6

Update function names

fc396ca

BlythMeister added 3 commits July 2, 2018 11:14

Add Trace tags for Failure and Final targets

d1f78fb

Merge remote-tracking branch 'origin/release/next' into tracing

814de03

# Conflicts: # src/app/Fake.Core.Target/Target.fs # src/app/Fake.Core.Trace/TraceListener.fs

Fix function naming issue

31c7b36

Remove verbose trace line

c515ca8

I have over 100 projects in a repo...this spamming 100 log lines isn't that useful. If this level is required, then you can trace around the calls to save the assembly info

matthid reviewed Jul 2, 2018

View reviewed changes

BlythMeister added 2 commits July 2, 2018 20:40

Update trace function to be a higher order function "useWith"

d471144

Use null rather than "NoDescription"

f56588a

BlythMeister force-pushed the tracing branch from 81045a7 to f56588a Compare July 2, 2018 19:48

BlythMeister added 2 commits July 3, 2018 08:58

Update descriptions to be string option

dbca42d

Update use with to switch based on the types of function called

a9b61bf

Simplify implementation so you always have access to the trace in you…

1843d82

…r function

matthid merged commit 2bd5909 into fsprojects:release/next Jul 4, 2018

BlythMeister mentioned this pull request Jul 5, 2018

Fixes To Target Traces #2016

Merged

BlythMeister deleted the tracing branch May 11, 2021 15:58

Tracing Updates #2009

Tracing Updates #2009

Conversation

BlythMeister commented Jun 29, 2018

matthid commented Jun 29, 2018

BlythMeister commented Jun 29, 2018

matthid commented Jun 29, 2018

BlythMeister commented Jun 29, 2018

matthid commented Jun 29, 2018

BlythMeister commented Jun 29, 2018

BlythMeister commented Jun 30, 2018 • edited Loading

BlythMeister commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

matthid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlythMeister commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

matthid commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

matthid commented Jul 2, 2018

BlythMeister commented Jul 2, 2018

BlythMeister commented Jul 3, 2018 • edited Loading

matthid commented Jul 3, 2018

BlythMeister commented Jul 3, 2018

BlythMeister commented Jul 4, 2018

matthid commented Jul 4, 2018

BlythMeister commented Jul 4, 2018

BlythMeister commented Jul 4, 2018

matthid commented Jul 4, 2018

BlythMeister commented Jul 4, 2018

matthid commented Jul 4, 2018

BlythMeister commented Jul 4, 2018

matthid commented Jul 4, 2018

BlythMeister commented Jun 30, 2018 •

edited

Loading

BlythMeister commented Jul 3, 2018 •

edited

Loading