Remove logrus #50829

rosstimothy · 2025-01-07T16:48:23Z

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/logrus

Includes https://github.com/gravitational/teleport.e/pull/5807
Depends on #50805

rosstimothy · 2025-01-08T14:20:40Z

lib/utils/log/handle_state.go

@@ -114,13 +113,6 @@ func (s *handleState) appendAttr(a slog.Attr) bool {
 				}
 			}
 			return nonEmpty
-		case logrus.Fields:


There is nothing including these fields in slog output so this is not needed.

rosstimothy · 2025-01-08T14:20:56Z

lib/utils/log/levels.go

This was moved into utils/log/slog.go

rosstimothy · 2025-01-08T14:21:22Z

lib/utils/log/logrus_formatter.go

The items deleted were moved into lib/utils/log/slog.go to facilitate removing this file in the future.

rosstimothy · 2025-01-08T14:21:59Z

tool/tbot/spiffe.go

 )

+// TODO(tross/noah): Remove once go-spiff has a slog<->workloadapi.Logger adapter.


FYI @strideynet. If you have a better solution to this in the short term I'm all ears.

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!

codingllama

Nice! We are almost 100% logrus-free!

.golangci.yml

e

lib/utils/log/slog.go

codingllama · 2025-01-08T18:40:17Z

tool/tbot/spiffe.go

+	//nolint:sloglint // msg cannot be constant
+	l.l.DebugContext(context.Background(), fmt.Sprintf(format, args...))


Maybe:

l.log(slog.LevelDebug, format, args...)

and implement the l.log function so it centralized the nolint / fmt.Sprintf logic?

codingllama · 2025-01-08T18:41:20Z

tool/tbot/spiffe.go

 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()}),


Maybe use a specialized component for this logger, or some other distinctive With key/value?

@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.

Happy to keep it as-is if you'd rather avoid other changes here.

codingllama · 2025-01-08T18:42:09Z

tool/teleport/common/teleport.go

@@ -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{}))


Is it a problem that these aren't equivalent anymore?

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?

No strong opinions on my part but attempting syslog first does seem better from a behavioral-compatibility point of view.

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.

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 added the no-changelog Indicates that a PR does not require a changelog entry label Jan 7, 2025

rosstimothy force-pushed the tross/config_slog branch from 55173a4 to 3a6dea2 Compare January 7, 2025 17:46

rosstimothy force-pushed the tross/remove_config_logrus branch 2 times, most recently from 737af4b to 7bcf7bb Compare January 7, 2025 18:44

Base automatically changed from tross/config_slog to master January 7, 2025 20:29

rosstimothy force-pushed the tross/remove_config_logrus branch 5 times, most recently from 0a31c37 to 9ed50e3 Compare January 8, 2025 14:19

rosstimothy commented Jan 8, 2025

View reviewed changes

rosstimothy force-pushed the tross/remove_config_logrus branch 4 times, most recently from d02cea6 to f9c7fa5 Compare January 8, 2025 17:47

rosstimothy marked this pull request as ready for review January 8, 2025 18:05

github-actions bot added machine-id size/sm tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Jan 8, 2025

github-actions bot requested review from bl-nero and strideynet January 8, 2025 18:06

rosstimothy requested a review from codingllama January 8, 2025 18:09

codingllama reviewed Jan 8, 2025

View reviewed changes

rosstimothy force-pushed the tross/remove_config_logrus branch from f9c7fa5 to e0433c8 Compare January 8, 2025 21:27

rosstimothy added 2 commits January 8, 2025 18:38

Refactor logger initialization

95ca8b2

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.

fix: document ValidateFields

839b31b

rosstimothy force-pushed the tross/remove_config_logrus branch from c31858f to 839b31b Compare January 8, 2025 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove logrus #50829

Remove logrus #50829

rosstimothy commented Jan 7, 2025 •

edited

Loading

rosstimothy Jan 8, 2025

rosstimothy Jan 8, 2025

rosstimothy Jan 8, 2025

rosstimothy Jan 8, 2025

strideynet Jan 8, 2025 •

edited

Loading

codingllama left a comment

codingllama Jan 8, 2025

codingllama Jan 8, 2025

rosstimothy Jan 8, 2025

codingllama Jan 8, 2025

codingllama Jan 8, 2025

rosstimothy Jan 8, 2025

codingllama Jan 8, 2025

rosstimothy Jan 8, 2025 •

edited

Loading

		)

		// TODO(tross/noah): Remove once go-spiff has a slog<->workloadapi.Logger adapter.

		//nolint:sloglint // msg cannot be constant
		l.l.DebugContext(context.Background(), fmt.Sprintf(format, args...))

Remove logrus #50829

Are you sure you want to change the base?

Remove logrus #50829

Conversation

rosstimothy commented Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strideynet Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

rosstimothy commented Jan 7, 2025 •

edited

Loading

strideynet Jan 8, 2025 •

edited

Loading

rosstimothy Jan 8, 2025 •

edited

Loading