exceptionless / Exceptionless.Net

Exceptionless clients for the .NET platform
https://exceptionless.com
Other
554 stars 142 forks source link

MinimumLevel setting is ignored while submitting the logs to dashboard #260

Closed jbulowski closed 1 year ago

jbulowski commented 3 years ago

Hello,

first of all i would like to apologize that I'm posting this issue here, but the serilog exceptionless extension project doesn't include the issues page.

I'm initializing logger with serilog like following

Log.Logger = new LoggerConfiguration()
                .MinimumLevel.Verbose()
                .WriteTo.Exceptionless(exceptionlessKey, exceptionlessUrl)
                .WriteTo.File(@"c:\logs\temp_logs.txt")
                .CreateLogger();

and while all the logs appear in the log file, I don't get logs below the Warning level in my dashboard.

I am using a self hosted version of excetionless and found the line that couses this

https://github.com/serilog/serilog-sinks-exceptionless/blob/master/src/Serilog.Sinks.Exceptionless/Sinks/Exceptionless/ExceptionlessClientExtensions.cs#L33

On this line the Constants.SourceContextPropertyName isn't getting found and because of it, it defaults to Warning and bypasses the inital setting (in Exceptionless.Models.SettingsDictionary.GetMinLogLevel()).

How do I fix it?

Thanks in advance

niemyjski commented 3 years ago

That's really weird that there is no issues showing there, I'm not sure if the serilog team turned that off or not. I think you may be running into the clients default minimum log level until server settings are set: https://github.com/exceptionless/Exceptionless.Net/blob/master/src/Exceptionless/Configuration/ExceptionlessConfiguration.cs#L209-L216 Does calling this resolve this issue? Also do you have a default log level set in project settings?

I'd also look into why your SourceContextPropertyName isn't being set. Are you only injecting ILogger orILogger<T>? Or are you not passing a name to ILoggerFactory.CreateLogger("name")?

jbulowski commented 3 years ago

Calling SetDefaultMinLogLevel doesn't change anything. And I'm only injecting ILogger like it's shown in the code snippet.

There is no settings in project setting for exceptionless. On the server side, the MinimumLevel is set to Debug.

niemyjski commented 3 years ago

Can you try enabling client logging and seeing what could be happening: https://exceptionless.com/docs/clients/dotnet/troubleshooting/

jbulowski commented 3 years ago

I've enabled client logging and it's not logging anything into the specified file. What I've found is that with the version 3.1.3 of the Serilog.Sinks.Exceptionless package everything works fine. I get the Debug and Information logs like I did before I updated the package, so I'm just gonna stick with this version. I guess you can close this issue.

Thanks for quick responses though.

niemyjski commented 3 years ago

Can you do a diff of that sink with the next version so we can know what's changed for you. It would be great to figure this out and get you on the latest version. Others may be running into it as well.

niemyjski commented 3 years ago

Looks like this is the only commit which changed deps: https://github.com/serilog/serilog-sinks-exceptionless/commit/ee28d4a77ce1759d9ab44c72c5f16538c14932b2 and here is all the client changes: https://github.com/exceptionless/Exceptionless.Net/compare/v4.5.0...v4.6.2 (lots of changes). Any chance you could upgrade and enable the serilog self logger. Also if you could reproduce in a small sample app of ours I'd be more than happy to debug it.

yuefengkai commented 2 years ago

https://github.com/exceptionless/Exceptionless.Net/issues/260#issuecomment-893231645 I also encountered the same problem, using Net6 miniApi cannot write logs below Warn level No such problem with 3.1.3

niemyjski commented 2 years ago

@yuefengkai Can you try this https://github.com/exceptionless/Exceptionless.Net/issues/259#issuecomment-862344080

ZhangYiQiu commented 2 years ago

I'm having the same problem, WriteTo.Exceptionless doesn't work. It resumed working after I manually set ExceptionlessClient.Default.Configuration.SetDefaultMinLogLevel(Exceptionless.Logging.LogLevel.Info);. But I think it should automatically use the MinimumLevel configured in appsettings.json without me having to manually SetDefaultMinLogLevel

"Serilog": {
    "MinimumLevel": {
      "Default": "Information"
    }
}

The version I'm using

.Net6 MVC
Exceptionless.AspNetCore 4.8.0
Serilog.Sinks.Exceptionless 3.1.5
niemyjski commented 2 years ago

@ZhangYiQiu Would you be willing to submit a pr for this behavior change here (https://github.com/exceptionless/serilog-sinks-exceptionless)

niemyjski commented 1 year ago

@ejsmith Any objections for trying to pick up the default log level from different logging providers and just using it for SetDefaultMinLogLevel? Last call to SetDefaultMinLogLevel wins.

ejsmith commented 1 year ago

I think you want to control these things separately, but agree that it is confusing. I think just because I want to see info level logging in my application log doesn't mean that I want to send all info level logs to Exceptionless.

niemyjski commented 1 year ago

But I think the general consensus of end users is (and myself) I just want to see my logs and I configured an exceptionless logging target so I'd expect to see all of my logs I send to my specific log provider in exceptionless.

ejsmith commented 1 year ago

Yeah, I guess, but it can also blow up your account real quick and then make our service useless. That is a big concern for me. How about we require a log level setting when you call .WriteTo.Exceptionless(exceptionlessKey, exceptionlessUrl, SomeLogLevelEnum.Auto) and you can then opt to use a setting that just sends everything that the logger gets and lets you control it through normal log level settings or you can explicitly set a minimum level that is specific to Exceptionless.

niemyjski commented 1 year ago

I'm not sure I've seen any other targets do that. I think we just keep it simple, if you are using logging target you expect lots of logs.

ejsmith commented 1 year ago

But I don't expect a lot of logs. I want to use the same mechanism that I already send errors to throughout my application (logging), but just because I want to output info level messages to my console doesn't mean I want that going to Exceptionless.

nivalxer commented 1 year ago

I'm not sure I've seen any other targets do that. I think we just keep it simple, if you are using logging target you expect lots of logs.

If using Serilog integration, the expected MinimumLevel should follow Serilog's configuration. In fact, there is a restrictedToMinimumLevel parameter in the initialization code of Serilog's Sinks, and the Sinks will use this parameter to determine if the minimum log level is met.

"Serilog": {
    "Using": [ "Serilog.Sinks.ExceptionLess" ],
    "MinimumLevel": {
      //global minimumLevel
      "Default": "Debug",
      "Override": {
        "Microsoft": "Warning",
        "System": "Warning"
      }
    },
    "WriteTo": [
      {
        "Name": "Exceptionless",
        "Args": {
          "apiKey": "xxxxxxxx",
          "serverUrl": "http://xxxxxx/"
          //If you want to specify a specific level
          "restrictedToMinimumLevel": "Error"
        }
      }
    ]
  }

Serilog will first use the global MinimumLevel to filter logs and then submit the logs to all configured Sinks. If the Sinks have a specific restrictedToMinimumLevel configuration, they will filter at the Emit stage based on this setting. So,in Serilog.Sinks.ExceptionLess, it is recommended to ignore the default configuration of ExceptionLessClient and just use Serilog's configuration. https://github.com/exceptionless/serilog-sinks-exceptionless/pull/16

niemyjski commented 1 year ago

@nivalxer I was thinking that there is another side of this that we may not be considering.

  1. The client has real time log levels and perhaps we had that check in your pr to save additional work from it even creating allocations and other work if we knew we were going to discard it. But then again, I'm thinking that perhaps the log targets/sinks should be dumber and leave that up to the client.
  2. I wonder if we should be looking at the target restrictedToMinimumLevel command and updating the client.Configuration.SetDefaultMinLogLevel This really only comes into effect when we have logs sent to us via the client outside of the sink. I think we are trying to be smart about outside of the target but the target should do what's it told.
     /// <summary>
        /// Set the default minimum log level that will be used until settings are loaded from the server.
        /// </summary>
        /// <param name="minLogLevel"></param>
        public void SetDefaultMinLogLevel(LogLevel minLogLevel) {
            if (!Settings.ContainsKey("@@log:*"))
                Settings.Add("@@log:*", minLogLevel.ToString());
        }
nivalxer commented 1 year ago

@niemyjski 1.My thinking is that if Serilog is considered the primary logging component, then all Sinks should follow Serilog's configuration. In fact, this is how Serilog's internal mechanism works as well: logs will first go through the global configuration's log level filtering, and then be submitted to each Sink in a loop, so the logs have already been filtered once. As considered, Sinks should be more simple-minded. I also referred to other Sinks, such as Serilog.Sinks.File, Serilog.Sinks.ApplicationInsights, etc., which all operate in this way, i.e., no need to check log levels again and submit directly. 2.I think there is no need to call client.Configuration.SetDefaultMinLogLevel to update the client's log level, unless the user explicitly needs to directly call the client to complete the log submission. If this is necessary, it is recommended that the user maintain a separate client instance.

ejsmith commented 1 year ago

I'm not sure I've seen any other targets do that. I think we just keep it simple, if you are using logging target you expect lots of logs.

If using Serilog integration, the expected MinimumLevel should follow Serilog's configuration. In fact, there is a restrictedToMinimumLevel parameter in the initialization code of Serilog's Sinks, and the Sinks will use this parameter to determine if the minimum log level is met.


"Serilog": {

    "Using": [ "Serilog.Sinks.ExceptionLess" ],

    "MinimumLevel": {

      //global minimumLevel

      "Default": "Debug",

      "Override": {

        "Microsoft": "Warning",

        "System": "Warning"

      }

    },

    "WriteTo": [

      {

        "Name": "Exceptionless",

        "Args": {

          "apiKey": "xxxxxxxx",

          "serverUrl": "http://xxxxxx/"

          //If you want to specify a specific level

          "restrictedToMinimumLevel": "Error"

        }

      }

    ]

  }

Serilog will first use the global MinimumLevel to filter logs and then submit the logs to all configured Sinks. If the Sinks have a specific restrictedToMinimumLevel configuration, they will filter at the Emit stage based on this setting.

So,in Serilog.Sinks.ExceptionLess, it is recommended to ignore the default configuration of ExceptionLessClient and just use Serilog's configuration.

https://github.com/exceptionless/serilog-sinks-exceptionless/pull/16

I didn't know about the restrictedToMinimumLevel feature. I agree that we should just make use of that feature and let Serilog do the filtering. We should make sure that we show that in our sample configuration so that people know how to change the level for just Exceptionless.

nivalxer commented 1 year ago

@niemyjski I looked at the source code and found that this issue was introduced in commit #fbce433262b101dbd4ca9d78e92f04e3bce4e37a, which changed the default log level from Trace to Warn. As a result, in Serilog.Sinks.ExceptionLess versions 3.1.3 later, if the default log level of the client is not explicitly set, logs below Warn cannot be submitted.

https://github.com/exceptionless/Exceptionless.Net/blob/7ed9cc3cb5a93e784cbf2518571bce88dc609219/src/Exceptionless/Models/Collections/SettingsDictionary.cs#L204

niemyjski commented 1 year ago

@nivalxer Yeah, which is why I was suggesting we take the min log level that the client is restricted to or the serilog level and call SetDefaultMinLogLevel. On a side note, for serilog looks like we need to bump the min framework version to 4.6.2 so we can update to latest .net client (prob would be good to update to .net 6 lts as well)

niemyjski commented 1 year ago

@nivalxer we have the SetDefaultMinLogLevel because we pull server-side logging levels that the client should submit (less data transfer from client and server processing) Until we have that log level or user hard codes it into client configuration, we use warning log level.

nivalxer commented 1 year ago

@niemyjski I agree. I have been using self-hosted ExceptionLess for many years. Our project uses Serilog as the default logging component, and in the production environment, we use Info-level logs to record the logs uploaded by IoT devices. The related configurations are all set using the AppSetting file method. I found that after upgrading Serilog.Sinks.ExceptionLess to 3.1.4, I could not record Info logs, so I was forced to roll back to version 3.1.3. Like other Sink configurations, we only set Serilog's default log level parameter. I also noticed that there are default log level related parameters sent by the server, but no parameters were sent in the default case. I also agree that using server-side configuration can reduce the transmission content. I am very much looking forward to the new version.Thank you very much.

niemyjski commented 1 year ago

@nivalxer @snakefoot would you mind submitting a pr to update this. Would be nice to have any logging implementations call SetDefaultMinLogLevel with the targets log level when the target is being configured.

nivalxer commented 1 year ago

@niemyjski hi,niemyjski.I've created a new PR to fix this issue, which now updates the client's minimum log level (defaulting to Trace if not specified) and also allows the use of the restrictedToMinimumLevel configuration in the Sink to update the client's minimum log level. https://github.com/exceptionless/serilog-sinks-exceptionless/pull/17

niemyjski commented 1 year ago

Thanks for the prs!

niemyjski commented 1 year ago

I've fixed the other providers in https://github.com/exceptionless/Exceptionless.Net/commit/580eea71a038e2fa9a1864e83ff466fefdc324d9 I also fixed one case in the NLog provider where it wasn't setting the default log level when using the default client instance.

niemyjski commented 1 year ago

Thank you all for your help, I'll create a new release once the backlog is emptied (soon)