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.

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

False positive on matching log args #69

Closed
Smaug123 opened this issue Jan 30, 2024 · 4 comments · Fixed by #71
Closed

False positive on matching log args #69

Smaug123 opened this issue Jan 30, 2024 · 4 comments · Fixed by #71
Assignees

Comments

@Smaug123
Copy link
Contributor

I observed today in GR that GRA-LOGTEMPLMISSVALS-001 incorrectly fired on the following:

log.LogWarning(e, "Error: {Something}", 3)

Since the first argument inherits from Exception, it should be ignored for the purposes of this analyzer.

@dawedawe
Copy link
Contributor

Mmh, I can't reproduce this one with:

type MyEx () =
    inherit System.Exception()

let testlog1 () =
    use factory = LoggerFactory.Create(fun b -> b.AddConsole() |> ignore)
    let log: ILogger = factory.CreateLogger("Program")
    let e: MyEx = MyEx()
    log.LogWarning(e, "Error: {Something}", 3)

Or do you mean it should ignore missing args if the first arg inherits from exception, like:

log.LogWarning(e, "Error: {Something}") // don't warn here

@Smaug123
Copy link
Contributor Author

Oh sorry, I'll try and reproduce it internally and get an exact repro out to you.

@dawedawe
Copy link
Contributor

Cool, thanks!

@nojaf
Copy link
Contributor

nojaf commented Jan 31, 2024

@dawedawe sample to reproduce should be:

namespace Blah

open Microsoft.Extensions.Logging

module Program =
    [<EntryPoint>]
    let main argv =
        let log : ILogger = failwith "TODO"
        try
            failwith "no"
        with e ->
            log.LogError (e, "Failed to look up: {s_Group}", 3)
            raise e
        0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants