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

Use config to control log report level and enrich log message #549

Merged
merged 4 commits into from
Mar 19, 2023

Conversation

inversionhourglass
Copy link
Contributor

Please answer these questions before submitting pull request

  • Why submit this pull request?

  • Bug fix

  • New feature provided

  • Improve performance

  • Related issues


Bug fix

  • Bug description.

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.
  1. Append Exception detail into log message if exception is not null.
  2. Now we can use config key SkyWalking::LogCollector::Level to control log collect level. It's useful if someone does not wanna collect log data or only want to collect error log data.
{
  "SkyWalking": {
    "LogCollector": {
      "Level": "Information" // default
    }
  }
}

@liuhaoyang
Copy link
Collaborator

For the configuration of the Diagnostics plugins, it is recommended to use the Diagnostics subsection.

{
  "SkyWalking": {
    "Diagnostics": {
      "Logging": {
          "CollectLevel": "Information"
       }
    }
  }
}

@inversionhourglass
Copy link
Contributor Author

Log is a top level data type same as trace and metric. Uses a top level configuration section maybe better. All diagnostics component of logger(mslogging, serilog, etc..) should implement this config by themself.

@liuhaoyang
Copy link
Collaborator

Log is a top level data type same as trace and metric. Uses a top level configuration section maybe better. All diagnostics component of logger(mslogging, serilog, etc..) should implement this config by themself.

The core only provides the ability to report logs to sw, and collect logs through the Diagnostics.Logging package, configuration should also be associated with Diagnostics.Logging.

@inversionhourglass
Copy link
Contributor Author

I'll change the code. But I still have reservations. For core concepts such as log, some basic configurations can be defined before the specific implementation. These configurations depend on the concept rather than the implementation.


代码我会按你的建议来改,但我仍然保留我的意见。像log这种核心概念,在具体实现之前就可以定义一些基本配置,这些配置是依赖于概念而不是依赖于实现。

Copy link
Collaborator

@liuhaoyang liuhaoyang left a comment

Choose a reason for hiding this comment

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

LGTM

@liuhaoyang liuhaoyang merged commit 606a14d into SkyAPM:main Mar 19, 2023
@wu-sheng wu-sheng added this to the 2.2.0 milestone Mar 19, 2023
@wu-sheng wu-sheng added the enhancement New feature or request label Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants