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

logflags: simplify Logger interface #3274

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Conversation

aarzilli
Copy link
Member

Remove methods we never used or used only very sparingly (Print) and we
probably shouldn't be using at all (Fatal).

@aarzilli
Copy link
Member Author

cc @blaubaer

@blaubaer
Copy link
Contributor

blaubaer commented Feb 15, 2023

I like the idea to have a much easier interface in this step.

Although currently nowhere structured logging is used, should we really remove:

  1. WithField(key string, value interface{}) Logger
  2. WithFields(fields Fields) Logger
  3. WithError(err error) Logger

...? If we leave it, it at least is a possibility to use it in a structured way. Maybe it might be a good idea to encourage it more?

@aarzilli
Copy link
Member Author

We do use it, but only in the constructor (makeLogger). We could keep WithFields and WithError, the fact that we haven't used it in the past 7 years suggests we will not use it in the future either and we can always add it later if we do need it (we are not making claims about api stability in this package). We could also do copy slog interface instead for this (possibly switching to slog).

@blaubaer
Copy link
Contributor

The question is why it wasn't used before. I believe if we (and I could help) simply add with this (or a change later) also usages of those methods (as role models) than they might be also used in the future. In this context we should maybe focus at first on the question is it worth it to strive for structured logging or not (but I would ignore in this context that it was not used before or not) and in the second step (if we believe it makes sense) how to make it happen. If we believe it does not worth it at all, we can drop it.

For the first question, my own opinion might be: Structured logging introduced always an better way to interpret log output as a human but also as machines (if it is done consequently).

@aarzilli
Copy link
Member Author

the question is it worth it to strive for structured logging or not

I don't know, if you want my opinion: it isn't beyond what we are doing now.

@blaubaer
Copy link
Contributor

I don't know, if you want my opinion: it isn't beyond what we are doing now.

For sure I'm looking for your opinion. But I can also live with it (as you say that we do not guarantee the stability of the public API of this package) to remove those methods for now and postpone that decision to later.

@derekparker
Copy link
Member

We could also do copy slog interface instead for this (possibly switching to slog).

I'm happy to move to slog, actually, and remove the burden of maintenance from us.

@aarzilli
Copy link
Member Author

I'm happy to move to slog, actually, and remove the burden of maintenance from us.

We can't move to slog for a few years due to backwards compatibility reasons (maybe sooner if the forward compatibility thing go 1.21 is doing works really well).

Remove methods we never used or used only very sparingly (Print) and we
probably shouldn't be using at all (Fatal).
Copy link
Member

@derekparker derekparker left a comment

Choose a reason for hiding this comment

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

LGTM

@derekparker derekparker merged commit 2b785f2 into go-delve:master Aug 7, 2023
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