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

Added std type to logging config. #2060

Merged
merged 7 commits into from
Dec 16, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions dendrite-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -370,9 +370,10 @@ tracing:
baggage_restrictions: null
throttler: null

# Logging configuration, in addition to the standard logging that is sent to
# stdout by Dendrite.
# Logging configuration
logging:
- type: std
level: info
- type: file
level: info
params:
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ require (
github.com/Arceliar/ironwood v0.0.0-20210619124114-6ad55cae5031
github.com/DATA-DOG/go-sqlmock v1.5.0
github.com/HdrHistogram/hdrhistogram-go v1.0.1 // indirect
github.com/MFAshby/stdemuxerhook v1.0.0 // indirect
github.com/Masterminds/semver/v3 v3.1.1
github.com/Shopify/sarama v1.29.1
github.com/codeclysm/extract v2.2.0+incompatible
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ github.com/HdrHistogram/hdrhistogram-go v1.0.1 h1:GX8GAYDuhlFQnI2fRDHQhTlkHMz8bE
github.com/HdrHistogram/hdrhistogram-go v1.0.1/go.mod h1:BWJ+nMSHY3L41Zj7CA3uXnloDp7xxV0YvstAE7nKTaM=
github.com/Joker/hpp v1.0.0/go.mod h1:8x5n+M1Hp5hC0g8okX3sR3vFQwynaX/UgSOM9MeBKzY=
github.com/Kubuxu/go-os-helper v0.0.1/go.mod h1:N8B+I7vPCT80IcP58r50u4+gEEcsZETFUpAzWW2ep1Y=
github.com/MFAshby/stdemuxerhook v1.0.0 h1:1XFGzakrsHMv76AeanPDL26NOgwjPl/OUxbGhJthwMc=
github.com/MFAshby/stdemuxerhook v1.0.0/go.mod h1:nLMI9FUf9Hz98n+yAXsTMUR4RZQy28uCTLG1Fzvj/uY=
github.com/Masterminds/semver/v3 v3.1.1 h1:hLg3sBzpNErnxhQtUy/mmLR2I9foDujNK030IGemrRc=
github.com/Masterminds/semver/v3 v3.1.1/go.mod h1:VPu/7SZ7ePZ3QOrcuXROw5FAcLl4a0cBrbBpGY/8hQs=
github.com/Microsoft/go-winio v0.4.11/go.mod h1:VhR8bwka0BXejwEJY73c50VrPtXAaKcyvVC4A4RozmA=
Expand Down
15 changes: 0 additions & 15 deletions internal/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,21 +91,6 @@ func SetupPprof() {
}
}

// SetupStdLogging configures the logging format to standard output. Typically, it is called when the config is not yet loaded.
func SetupStdLogging() {
logrus.SetReportCaller(true)
logrus.SetFormatter(&utcFormatter{
&logrus.TextFormatter{
TimestampFormat: "2006-01-02T15:04:05.000000000Z07:00",
FullTimestamp: true,
DisableColors: false,
DisableTimestamp: false,
QuoteEmptyFields: true,
CallerPrettyfier: callerPrettyfier,
},
})
}

// File type hooks should be provided a path to a directory to store log files
func checkFileHookParams(params map[string]interface{}) {
path, ok := params["path"]
Expand Down
26 changes: 26 additions & 0 deletions internal/log_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@
package internal

import (
"io/ioutil"
"log/syslog"

"github.com/MFAshby/stdemuxerhook"
"github.com/matrix-org/dendrite/setup/config"
"github.com/sirupsen/logrus"
lSyslog "github.com/sirupsen/logrus/hooks/syslog"
Expand All @@ -29,6 +31,17 @@ import (
// so we just exit with the error
func SetupHookLogging(hooks []config.LogrusHook, componentName string) {
logrus.SetReportCaller(true)
logrus.SetFormatter(&utcFormatter{
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this back to internal/log.go and undo the changes to setup/base/base.go, I'm happy with this. :)

&logrus.TextFormatter{
TimestampFormat: "2006-01-02T15:04:05.000000000Z07:00",
FullTimestamp: true,
DisableColors: false,
DisableTimestamp: false,
QuoteEmptyFields: true,
CallerPrettyfier: callerPrettyfier,
},
})
stdLogAdded := false
for _, hook := range hooks {
// Check we received a proper logging level
level, err := logrus.ParseLevel(hook.Level)
Expand All @@ -49,10 +62,19 @@ func SetupHookLogging(hooks []config.LogrusHook, componentName string) {
case "syslog":
checkSyslogHookParams(hook.Params)
setupSyslogHook(hook, level, componentName)
case "std":
setupStdLogHook(level)
stdLogAdded = true
default:
logrus.Fatalf("Unrecognised logging hook type: %s", hook.Type)
}
}
if !stdLogAdded {
logrus.Info("No std logger config found. Enabling at INFO level by default")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure if we need to log this? It just seems like noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I was pretty much just using it for a quick test. Could take it out.

setupStdLogHook(logrus.InfoLevel)
}
// Hooks are now configured for stdout/err, so throw away the default logger output
logrus.SetOutput(ioutil.Discard)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm somewhat wondering whether something like logrus.SetLevel(logrus.PanicLevel) is better here, since otherwise we're allocating log buffers just to write them to a discarding writer, which seems like a waste of allocations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure: wouldn't this set the level for all loggers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure to be honest. Just a thought / might be worth trying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work, I'm afraid. Calling SetLevel(logrus.PanicLevel) simply disables all logging

This explains why this code here is required:

if logrus.GetLevel() < level {

I think this is just a limitation of logrus.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's unfortunate. We'll just have to go with io.Discard anyway then.

}

func checkSyslogHookParams(params map[string]interface{}) {
Expand All @@ -76,6 +98,10 @@ func checkSyslogHookParams(params map[string]interface{}) {

}

func setupStdLogHook(level logrus.Level) {
logrus.AddHook(&logLevelHook{level, stdemuxerhook.New(logrus.StandardLogger())})
}

func setupSyslogHook(hook config.LogrusHook, level logrus.Level, componentName string) {
syslogHook, err := lSyslog.NewSyslogHook(hook.Params["protocol"].(string), hook.Params["address"].(string), syslog.LOG_INFO, componentName)
if err == nil {
Expand Down
1 change: 0 additions & 1 deletion setup/base/base.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ func NewBaseDendrite(cfg *config.Dendrite, componentName string, options ...Base
logrus.Fatalf("Failed to start due to configuration errors")
}

internal.SetupStdLogging()
internal.SetupHookLogging(cfg.Logging, componentName)
internal.SetupPprof()

Expand Down