Getting rid of sirupsen/logrus and log.Interface #359

duricanikolic · 2023-08-13T12:57:38Z

What this PR does:
Recently some parts of weaveworks/common have been migrated to dskit (PR). That migration included into dskit also the log.Interface interface that 'unifies' gokit logging and logrus logging.
On the other hand, all Loki, Tempo, Mimir and Pyroscope use only gokit, and neither from them uses logrus directly. Therefore, this PR removes the usage of lrogus and log.Interface from dskit.

Checklist

Tests updated
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

I just did a very high level pass.

log/global.go

log/format.go

log/gokit.go

middleware/logging.go

user/logging.go

middleware/logging.go

zalegrala · 2023-08-14T15:51:30Z

Are users meant to use go-kit instead of the log.Interface here? I was looking in this area the other day and wondering which pattern users were meant to use.

middleware/grpc_logging.go

log/gokit.go

log/logging.go

log/gokit_test.go

log/gokit.go

duricanikolic · 2023-08-14T19:07:15Z

Are users meant to use go-kit instead of the log.Interface here? I was looking in this area the other day and wondering which pattern users were meant to use.

Hi @zalegrala,
The main goal of this PR is to remove the log.Interface interface, and the usage of logrus, so the only supported logger would be go-kit Logger.

I will ask all the teams that use dskit what they think about this idea, and if they all agree, we can merge this PR.

simonswine

I am okay with the proposed change, but in case we wanted to appeal to projects (external/internal) which don't use log-kit, this interface is useful and in theory could be developed into compatibility with slog, which is in the stdlib from 1.21.

In any way I am not seeing any blockers from a consumer perspective of grafana/pyroscope

duricanikolic · 2023-08-15T08:28:25Z

I am okay with the proposed change, but in case we wanted to appeal to projects (external/internal) which don't use log-kit, this interface is useful and in theory could be developed into compatibility with slog, which is in the stdlib from 1.21.

In any way I am not seeing any blockers from a consumer perspective of grafana/pyroscope

Hi @simonswine,
Thank you for the feedback. Your remark about keeping log.Interface because it is compatible with other loggers sounds interesting. For the time being no project actually uses it, so I thought about removing it. To be more clear, since all Loki, Tempo, Pyroscope and Mimir use `go-kit' Logger, instead of doing

logger := GoKit(log) // this wraps the go-kit logger into log.Interface
logger.Infof("There are %d projects", 4)

they simply do (not exactly, this is just an example)

level.Info(log).Log("msg", fmt.Sprintf("There are %d projects", 4))

So, we can, for the moment, remove the interface, and if at a point there will be a need to move to another logger (e.g., slog) we will revert the change and put log.Interface back. That change will not be breaking, and projects will not need to change anything, unless they want to migrate to log.Interface. WDYT?

simonswine · 2023-08-15T10:02:15Z

@duricanikolic the original ambition for dskit was to be generic and adopted inside and outside of Grafana Labs by people building distributed services. This is obviously not the reality today (and might never be). This is in no way blocking or a strongly held opinion, but I think this gets us further from that vision.

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic · 2023-08-16T05:58:41Z

This branch show the changes needed to be done in Loki if this draft PR gets merged.

This branch show the changes needed to be done in Tempo if this draft PR gets merged.

duricanikolic · 2023-08-16T06:22:27Z

@duricanikolic the original ambition for dskit was to be generic and adopted inside and outside of Grafana Labs by people building distributed services. This is obviously not the reality today (and might never be). This is in no way blocking or a strongly held opinion, but I think this gets us further from that vision.

@simonswine if we decide to keep log.Interface how feasible do you think it is that the teams will actually decide to use it and to switch from gokit.Logger to log.Interface?

For example, all occurrences of

level.Info(logger).Log(...)

will have to be replaced with

wrappedLog := GoKit(logger) // wraps gokit.Logger into log.Interface
wrappedLog.Infoln(...)

Moreover, the interface has some disadvantages: for example, if we want to log

level.Info(logger).Log("key1", "value1", "key2", "value2", "msg", "abc")

we need to do

wrappedLog.WithField("key1", "value1").WithField("key2", "value2").Infoln("abc") // Infoln and Infof implicitly add the "msg" key and print the argument as its value

but the calls to WithField are expensive.

pracucci · 2023-08-16T12:54:02Z

I see @simonswine comment about the log.Interface, however I would like to share different perspective.

This PR is not a regression in that regards: dskit doesn't use log.Interface. The interface was imported 2 weeks ago when copying weaveworks/common into dskit (PR). Dskit itself wasn't using the interface before and neither after, given the interface is only used by the code copied from weaveworks/common.

Do we want to be less opinionated and support a generic log interface for all the logging done by dskit? Maybe. Is this the main blocker to dskit wide adoption? I doubt (e.g. the lack of any doc may be a bigger blocker). Personally, I would defer the discussion about a generic logging interface until we'll have a proof that's the blocker for an adoption outside Grafana Labs.

log/logging.go

log/ratelimit.go

middleware/grpc_logging.go

middleware/path_rewrite.go

log/gokit.go

Signed-off-by: Yuri Nikolic <[email protected]>

pracucci

LGTM, modulo one last fix

server/server.go

Signed-off-by: Yuri Nikolic <[email protected]>

zalegrala

This looks reasonable to me.

duricanikolic · 2023-08-16T20:53:23Z

This looks reasonable to me.

@zalegrala could you please approve the PR?

trevorwhitney

Looks good from Loki team. Thanks!

log/gokit.go

Signed-off-by: Yuri Nikolic <[email protected]>

pracucci

I hope I haven't missed anything. LGTM! (I just left a last nit)

log/gokit.go

Signed-off-by: Yuri Nikolic <[email protected]>

…#10434) This PR removes unnecessary code used to convert the dereleased `dslog.Interface` (see grafana/dskit#359) into `go-kit.Logger`. Additionally, it modifies the signature and implementation of `pkg/util/log/log.go`'s `InitLogger` as follows: - previous signature ``` InitLogger(*server.Config, prometheus.Registerer, bool, bool) ``` has been replaced with ``` InitLogger(*server.Config, prometheus.Registerer, bool, bool) log.Logger ``` Namely, the new `InitLogger` initialises the global logger and returns it. - the new implementation initializes the global logger, but it does not override the default logger for the server. This is now done by the callers of `InitLogger`. Previously, it was done inside `InitLogger` for convenience, since the types of the 2 loggers were not the same. Signed-off-by: Yuri Nikolic <[email protected]>

…grafana#10434) This PR removes unnecessary code used to convert the dereleased `dslog.Interface` (see grafana/dskit#359) into `go-kit.Logger`. Additionally, it modifies the signature and implementation of `pkg/util/log/log.go`'s `InitLogger` as follows: - previous signature ``` InitLogger(*server.Config, prometheus.Registerer, bool, bool) ``` has been replaced with ``` InitLogger(*server.Config, prometheus.Registerer, bool, bool) log.Logger ``` Namely, the new `InitLogger` initialises the global logger and returns it. - the new implementation initializes the global logger, but it does not override the default logger for the server. This is now done by the callers of `InitLogger`. Previously, it was done inside `InitLogger` for convenience, since the types of the 2 loggers were not the same. Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic self-assigned this Aug 13, 2023

duricanikolic force-pushed the yuri/rate-limit-log branch 5 times, most recently from 7852e6d to 598d45b Compare August 13, 2023 17:17

pracucci reviewed Aug 14, 2023

View reviewed changes

duricanikolic requested a review from pracucci August 14, 2023 10:28

duricanikolic force-pushed the yuri/rate-limit-log branch 2 times, most recently from 62c7d10 to 1d4cd9e Compare August 14, 2023 13:57

pracucci reviewed Aug 14, 2023

View reviewed changes

simonswine approved these changes Aug 15, 2023

View reviewed changes

duricanikolic force-pushed the yuri/rate-limit-log branch from afd1df3 to d27fa49 Compare August 15, 2023 09:16

duricanikolic force-pushed the yuri/rate-limit-log branch 6 times, most recently from f564a9d to a995ee0 Compare August 15, 2023 23:02

duricanikolic added 4 commits August 16, 2023 01:19

Getting rid of sirupsen/logrus and log.Interface

2a7bd68

Signed-off-by: Yuri Nikolic <[email protected]>

Fixing review findings

999a956

Signed-off-by: Yuri Nikolic <[email protected]>

Fixing review findings 2

2810e40

Signed-off-by: Yuri Nikolic <[email protected]>

Add NewGoKitWithFields constructor

69fe22b

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic force-pushed the yuri/rate-limit-log branch from a995ee0 to 69fe22b Compare August 15, 2023 23:22

pracucci reviewed Aug 16, 2023

View reviewed changes

Fix review findings 3

1965c0c

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic requested a review from pracucci August 16, 2023 14:17

duricanikolic marked this pull request as ready for review August 16, 2023 14:19

This was referenced Aug 16, 2023

Upgrade dskit to the version without log.Interface and logrus grafana/tempo#2796

Draft

Upgrade to dskit without log.Interface and logrus grafana/loki#10265

Closed

pracucci approved these changes Aug 16, 2023

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

duricanikolic force-pushed the yuri/rate-limit-log branch from c6b3a4b to 3d505b9 Compare August 16, 2023 15:29

duricanikolic requested a review from trevorwhitney August 16, 2023 15:46

Fixing review findings 4

928f665

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic force-pushed the yuri/rate-limit-log branch from 3d505b9 to 928f665 Compare August 16, 2023 15:48

duricanikolic requested a review from zalegrala August 16, 2023 16:07

zalegrala reviewed Aug 16, 2023

View reviewed changes

trevorwhitney approved these changes Aug 16, 2023

View reviewed changes

pracucci reviewed Aug 17, 2023

View reviewed changes

log/gokit.go Outdated Show resolved Hide resolved

Adding NewGoKitWithWriter constructor

e6b0851

Signed-off-by: Yuri Nikolic <[email protected]>

pracucci approved these changes Aug 17, 2023

View reviewed changes

log/gokit.go Outdated Show resolved Hide resolved

log/gokit.go Outdated Show resolved Hide resolved

log/gokit.go Outdated Show resolved Hide resolved

Fixing review findings

f586a02

Signed-off-by: Yuri Nikolic <[email protected]>

duricanikolic merged commit 90d7ee0 into main Aug 17, 2023

duricanikolic deleted the yuri/rate-limit-log branch August 17, 2023 13:46

This was referenced Aug 17, 2023

Upgrade dskit and improve log.InitLogger function grafana/tempo#2799

Closed

Upgrade to dskit without log.Interface and logrus grafana/pyroscope#2268

Closed

Upgrade to dskit without log.Interface and logrus grafana/pyroscope#2270

Merged

duricanikolic mentioned this pull request Sep 4, 2023

Remove unnecessary code after dskit log.Interface has been dereleased grafana/loki#10434

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Getting rid of sirupsen/logrus and log.Interface #359

Getting rid of sirupsen/logrus and log.Interface #359

duricanikolic commented Aug 13, 2023 •

edited

Loading

pracucci left a comment

zalegrala commented Aug 14, 2023

duricanikolic commented Aug 14, 2023

simonswine left a comment

duricanikolic commented Aug 15, 2023

simonswine commented Aug 15, 2023

duricanikolic commented Aug 16, 2023

duricanikolic commented Aug 16, 2023

pracucci commented Aug 16, 2023

pracucci left a comment

zalegrala left a comment

duricanikolic commented Aug 16, 2023

trevorwhitney left a comment

pracucci left a comment

Getting rid of sirupsen/logrus and log.Interface #359

Getting rid of sirupsen/logrus and log.Interface #359

Conversation

duricanikolic commented Aug 13, 2023 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

zalegrala commented Aug 14, 2023

duricanikolic commented Aug 14, 2023

simonswine left a comment

Choose a reason for hiding this comment

duricanikolic commented Aug 15, 2023

simonswine commented Aug 15, 2023

duricanikolic commented Aug 16, 2023

duricanikolic commented Aug 16, 2023

pracucci commented Aug 16, 2023

pracucci left a comment

Choose a reason for hiding this comment

zalegrala left a comment

Choose a reason for hiding this comment

duricanikolic commented Aug 16, 2023

trevorwhitney left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

duricanikolic commented Aug 13, 2023 •

edited

Loading