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

Settable global logger #1348

Merged
merged 2 commits into from
Nov 1, 2022
Merged

Conversation

uablrek
Copy link
Contributor

@uablrek uablrek commented Sep 8, 2022

Description

It should be possible to set an own customizwd Logger to be used by NSM. Now the logger is hard-coded in some places, most notable the logruslogger.FromSpan() used for trace logging. The intention for this PR is to fix the most urgent needs in a backward compatible way with as little changes as possible.

This PR adds a global logger reachable with log.L() (same as zap) and settable with log.SetGlobalLogger(l Logger).

logruslogger.FromSpan() has been altered to become generic by using the log.Logger interface rather than a variation of logruslogger. It uses the global logger as base (calls log.L()) and thus allows users to specify the logger used for trace. For backward compatibility the global Logger is set to a logruslogger on init. If no logger is specified (logger is log.Default()) a backward compatible logger (logrus) is used.

The global logger should be set early in main(). Example;

	if config.LogLevel == "TRACE" {
		nsmlog.EnableTracing(true)
		// Work-around for hard-coded logrus dependency in NSM
		logrus.SetLevel(logrus.TraceLevel)
	}
	logger.Info("NSM trace", "enabled", nsmlog.IsTracingEnabled())
	nsmlogger := log.NSMLogger(logger)
	nsmlog.SetGlobalLogger(nsmlogger)
	ctx = nsmlog.WithLog(ctx, nsmlogger)

Tasks that should be done but are not backward compatible

  • Move FromSpan() from package logruslogger to log (since it's not logrus specific any more)
  • Rename FromSpan() -> NewTraceLogger()

I will not add these things since it affects code all over the place I guess. Also FromSpan() is a horrible hack IMHO and should be re-written.

Issue link

#1272

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

I have used this PR in our NSM application with trace and it works fine. The example above is from real code. I have also used a logruslogger/logruslogger_test.go unit test to exercise the code in a convenient way. I have not included it since it doesn't really make any automatic tests, it just emits logs in various ways to be eyeballed by the developer.

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@uablrek
Copy link
Contributor Author

uablrek commented Sep 8, 2022

Example with a custom logger;

{
  "severity": "debug",
  "timestamp": "2022-09-08T18:13:49.429+00:00",
  "service_id": "Meridio-LB",
  "message": "(7)       ⎆ sdk/pkg/registry/core/trace/traceNetworkServiceEndpointRegistryFindClient.Recv()",
  "version": "1.0.0",
  "extra_data": {
    "subsystem": "NSM",
    "type": "registry"
  }
}

@uablrek
Copy link
Contributor Author

uablrek commented Sep 8, 2022

About failed CI;

  • I can't remove the init() function without breaking backward compatibility
  • The FromContext() now returns log.L() if there is no log in the contexts. IMHO this is the right way, but apparetly it breaks CI. I can change it back

The others seem to be minor updates. I can fix those.

@edwarnicke
Copy link
Member

@denis-tingaikin
Copy link
Member

@uablrek There is also failed build/tests. Could you have a look?

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Oct 25, 2022

I'm just interested in why we can't just declare a global context and put there a logger.

@uablrek Could you remind me why we can't use a global context?

@uablrek
Copy link
Contributor Author

uablrek commented Oct 25, 2022

Could you remind me why we can't use a global context?

Not sure, but nobody else does that AFAIK so I suppose there are reasons. It is recommended to pass context as the first parameter (not using a global context) and context's are more often derived than a logger.

@uablrek uablrek force-pushed the log-settable branch 2 times, most recently from 59a7e13 to c78978d Compare October 26, 2022 11:40
Lars Ekman added 2 commits October 28, 2022 14:02
Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
@edwarnicke edwarnicke merged commit fd7a204 into networkservicemesh:main Nov 1, 2022
nsmbot pushed a commit to networkservicemesh/cmd-cluster-info-k8s that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-map-ip-k8s that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-admission-webhook-k8s that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-ipam-vl3 that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-remote-vlan that referenced this pull request Nov 1, 2022
…k@main

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
denis-tingaikin pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Nov 8, 2022
…k@main (#551)

PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
  - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Co-authored-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/deployments-k8s that referenced this pull request Nov 8, 2022
…d-nsmgr@main

PR link: networkservicemesh/cmd-nsmgr#551

Commit: 4a90d30
Author: Network Service Mesh Bot
Date: 2022-11-08 03:02:24 -0600
Message:
  - Update go.mod and go.sum to latest version from networkservicemesh/sdk@main (#551)
PR link: networkservicemesh/sdk#1348

Commit: fd7a204
Author: Lars Ekman
Date: 2022-11-01 16:17:33 +0100
Message:
    - Settable global logger (#1348)
* Add a settable global logger

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>

* Use the global logger for trace-logging

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

Signed-off-by: Lars Ekman <lars.g.ekman@est.tech>
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
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 this pull request may close these issues.

3 participants