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

[FEATURE] Pretty-print logging #237

Closed
james-milligan opened this issue Dec 20, 2022 · 9 comments · Fixed by #322
Closed

[FEATURE] Pretty-print logging #237

james-milligan opened this issue Dec 20, 2022 · 9 comments · Fixed by #322
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers Needs Triage This issue needs to be investigated by a maintainer

Comments

@james-milligan
Copy link
Contributor

james-milligan commented Dec 20, 2022

Requirements

Flagd currently prints structured logs in json format, when running flagd in the console this can cause the logs to be difficult to read.

the zap logger zap.Config struct contains the field Encoding which when set to console instead of json updates the logging structure as follows:

json

{"level":"info","ts":1671548378.306176,"caller":"runtime/runtime.go:84","msg":"configuration change (write) for flagKey headerColor (config/samples/example_flags.json)","component":"runtime"}
{"level":"info","ts":1671548378.3061912,"caller":"runtime/runtime.go:84","msg":"configuration change (write) for flagKey myBoolFlag (config/samples/example_flags.json)","component":"runtime"}
{"level":"info","ts":1671548378.306196,"caller":"runtime/runtime.go:84","msg":"configuration change (write) for flagKey myStringFlag (config/samples/example_flags.json)","component":"runtime"}
{"level":"info","ts":1671548378.306199,"caller":"runtime/runtime.go:84","msg":"configuration change (write) for flagKey myFloatFlag (config/samples/example_flags.json)","component":"runtime"}
{"level":"info","ts":1671548378.3062022,"caller":"runtime/runtime.go:84","msg":"configuration change (write) for flagKey myIntFlag (config/samples/example_flags.json)","component":"runtime"}

console

1.671549411055416e+09   info    runtime/runtime.go:84   configuration change (write) for flagKey myFloatFlag (config/samples/example_flags.json)        {"component": "runtime"}
1.6715494110554419e+09  info    runtime/runtime.go:84   configuration change (write) for flagKey myIntFlag (config/samples/example_flags.json)  {"component": "runtime"}
1.671549411055447e+09   info    runtime/runtime.go:84   configuration change (write) for flagKey myObjectFlag (config/samples/example_flags.json)       {"component": "runtime"}
1.671549411055451e+09   info    runtime/runtime.go:84   configuration change (write) for flagKey isColorYellow (config/samples/example_flags.json)      {"component": "runtime"}

This behaviour should be controlled via a viper flag, e.g. --log-encoding=console with a json default e.g. --log-encoding=json with a console default

@james-milligan james-milligan added enhancement New feature or request Needs Triage This issue needs to be investigated by a maintainer labels Dec 20, 2022
@james-milligan james-milligan added the good first issue Good for newcomers label Jan 6, 2023
@mihirm21
Copy link
Member

Hi, can I be assigned this issue in case no one else is working on it, or should I directly submit my PR?

@mihirm21 mihirm21 mentioned this issue Jan 22, 2023
mihirm21 added a commit to mihirm21/flagd that referenced this issue Jan 22, 2023
[FEATURE] Pretty-print logging open-feature#237

Signed-off-by: Mihir Mittal <105881639+mihirm21@users.noreply.github.com>
mihirm21 added a commit to mihirm21/flagd that referenced this issue Jan 22, 2023
[FEATURE] Pretty-print logging open-feature#237

Signed-off-by: Mihir Mittal <105881639+mihirm21@users.noreply.github.com>
This was referenced Jan 22, 2023
@mihirm21
Copy link
Member

Is the issue resolved now ?

@james-milligan
Copy link
Contributor Author

Ive requested changes in the PR
#302 (review)

@mihirm21
Copy link
Member

mihirm21 commented Jan 25, 2023

are you asking for changes in source files of zap encoder? other than that, I am not getting where to add this feature, plz help

@james-milligan
Copy link
Contributor Author

james-milligan commented Jan 25, 2023

You can introduce a new cli flag in cmd/start.go, mirroring one of the other existing flags and setting the default to "console"
This flag value can be retrieved with viper.GetString(YOUR_FLAG_NAME) and passed into the constructor of the zap the logger setting the zap.Config Encoding property to the value passed on startup.

This way we set the default to console while still allowing for developers to use structured json logging if they require it

@mihirm21
Copy link
Member

mihirm21 commented Jan 25, 2023

how do I pass this value(viper.GetString(flag name)) to the zap.config encoding parameter

@james-milligan
Copy link
Contributor Author

james-milligan commented Jan 25, 2023

One option is to update the NewZapLogger signature to the following:

func NewZapLogger(level zapcore.Level, logFormat string) (*zap.Logger, error)

or

update the NewZapLogger signature to the following:

func NewZapLogger(config zap.Config) (*zap.Logger, error)

and apply the remaining default values within the NewZapLogger method, e.g.
start.go:

l, err := logger.NewZapLogger(zap.Config{
	Encoding:         viper.GetString(flag name),
	Level:                zap.NewAtomicLevelAt(level),
})

logger.go:

func NewZapLogger(cfg zap.Config) (*zap.Logger, error) {
        if cfg.Encoding == "" {
                cfg.Encoding = "console"
        }
        ...
        ...
        return cfg.Build()	
}

I am sure there are other options as well if you decide to take a different route

@mihirm21
Copy link
Member

Is the issue resolved now?

@beeme1mr
Copy link
Member

Hi @mihirm21, the issue will automatically be resolved once your PR is accepted and merged. Thanks for your contributions to the project!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Needs Triage This issue needs to be investigated by a maintainer
Projects
None yet
3 participants