Remove logrus #50829
Remove logrus #50829
Conversation
rosstimothy
added
the
no-changelog
rosstimothy
force-pushed
the
tross/config_slog
branch
from
55173a4 to
3a6dea2
Compare
rosstimothy
force-pushed
the
tross/remove_config_logrus
branch
2 times, most recently
from
737af4b to
7bcf7bb
Compare
rosstimothy
force-pushed
the
tross/remove_config_logrus
branch
5 times, most recently
from
0a31c37 to
9ed50e3
Compare
| @@ -114,13 +113,6 @@ func (s *handleState) appendAttr(a slog.Attr) bool { | |||
| } | |||
| } | |||
| return nonEmpty | |||
| case logrus.Fields: | |||
There was a problem hiding this comment.
There is nothing including these fields in slog output so this is not needed.
lib/utils/log/levels.go
Outdated
There was a problem hiding this comment.
This was moved into utils/log/slog.go
lib/utils/log/logrus_formatter.go
Outdated
There was a problem hiding this comment.
The items deleted were moved into lib/utils/log/slog.go to facilitate removing this file in the future.
| ) | ||
|
|
||
| // TODO(tross/noah): Remove once go-spiff has a slog<->workloadapi.Logger adapter. |
There was a problem hiding this comment.
FYI @strideynet. If you have a better solution to this in the short term I'm all ears.
There was a problem hiding this comment.
I have a ticket w/ go-spiffe to add slog support - spiffe/go-spiffe#281 - perhaps link this ticket here?
What you've done here is good for now!
rosstimothy
force-pushed
the
tross/remove_config_logrus
branch
4 times, most recently
from
d02cea6 to
f9c7fa5
Compare
github-actions
bot
added
machine-id
size/sm
tctl
| //nolint:sloglint // msg cannot be constant | ||
| l.l.DebugContext(context.Background(), fmt.Sprintf(format, args...)) |
There was a problem hiding this comment.
Maybe:
l.log(slog.LevelDebug, format, args...)and implement the l.log function so it centralized the nolint / fmt.Sprintf logic?
| func onSPIFFEInspect(ctx context.Context, path string) error { | ||
| log.InfoContext(ctx, "Inspecting SPIFFE Workload API Endpoint", "path", path) | ||
|
|
||
| source, err := workloadapi.New( | ||
| ctx, | ||
| // TODO(noah): Upstream PR to add slog<->workloadapi.Logger adapter. | ||
| workloadapi.WithLogger(utils.NewLogger()), | ||
| // https://github.com/spiffe/go-spiffe/issues/281 | ||
| workloadapi.WithLogger(logger{l: slog.Default()}), |
There was a problem hiding this comment.
Maybe use a specialized component for this logger, or some other distinctive With key/value?
There was a problem hiding this comment.
@strideynet any opinions here? There was no component set prior, so I'm inclined to leave it as is to reduce changing functionality too much here.
There was a problem hiding this comment.
Happy to keep it as-is if you'd rather avoid other changes here.
There was a problem hiding this comment.
I'm happy with this for now. I'll revisit all of tbot daemon logs at somepoint
tool/teleport/common/teleport.go
Outdated
| @@ -1017,7 +1017,7 @@ func dumpConfigFile(outputURI, contents, comment string) (string, error) { | |||
| func onSCP(scpFlags *scp.Flags) (err error) { | |||
| // when 'teleport scp' is executed, it cannot write logs to stderr (because | |||
| // they're automatically replayed by the scp client) | |||
| utils.SwitchLoggingToSyslog() | |||
| slog.SetDefault(slog.New(logutils.DiscardHandler{})) | |||
There was a problem hiding this comment.
Is it a problem that these aren't equivalent anymore?
There was a problem hiding this comment.
This will essentially prevent any logs from being emitted, though the comment here says nothing should be emitted to stderr, if something went wrong in SwitchLoggingToSyslog it would fallback to using stderr. I could at least attempt to output to syslog first and fall back to discarding if that doesn't work. What do you think?
There was a problem hiding this comment.
No strong opinions on my part but attempting syslog first does seem better from a behavioral-compatibility point of view.
There was a problem hiding this comment.
WDYT about 95ca8b2? I've shuffled the log initialization code around a bit to accomodate setting things up the same way for this command as others. I plan on also using this in a follow up PR to have the integrations use it instead of their duplicated variant.
The only remaining use of logrus is from integrations, which is unfortunately imported by teleport.e, and prevents logrus from being moved to an indirect dependency. The logrus formatter and initialization of the logrus logger will remain in place until integrations is using slog. To prevent any accidental inclusions of logrus within the teleport module the depguard rules have been updated to prohibit importing logrus. The rules also include prohibit a few common log packages that tools like gopls might automatically import.
rosstimothy
force-pushed
the
tross/remove_config_logrus
branch
from
f9c7fa5 to
e0433c8
Compare
Consolidates configuring of global loggers to a single function. This is mainly to facilitate configuring the logger for teleport scp, but will also allow us to remove the copy of logger initialization that currently exists in integrations/lib/logger.
rosstimothy
force-pushed
the
tross/remove_config_logrus
branch
from
c31858f to
839b31b
Compare
The only remaining use of logrus is from integrations, which is unfortunately imported by teleport.e, and prevents logrus from being moved to an indirect dependency. The logrus formatter and initialization of the logrus logger will remain in place until integrations is using slog. To prevent any accidental inclusions of logrus within the teleport module the depguard rules have been updated to prohibit importing logrus. The rules also include prohibit a few common log packages that tools like gopls might automatically import.
$ go mod why github.com/sirupsen/logrus # github.com/sirupsen/logrus github.com/gravitational/teleport/integrations/access/common/auth github.com/sirupsen/logrusIncludes https://github.com/gravitational/teleport.e/pull/5807
Depends on #50805