Address timr's feedback

gravitational · Oct 1, 2024 · c927143 · c927143
1 parent 158ee0f
commit c927143
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 29 deletions.
diff --git a/integrations/access/msteams/app.go b/integrations/access/msteams/app.go
@@ -60,15 +60,14 @@ type App struct {
 
 // NewApp initializes a new teleport-msteams app and returns it.
 func NewApp(conf Config) (*App, error) {
-	log, err := conf.Log.NewSLog()
+	log, err := conf.Log.NewSLogLogger()
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
 
-	log = log.With("plugin", pluginName)
 	app := &App{
 		conf: conf,
-		log:  log,
+		log:  log.With("plugin", pluginName),
 	}
 
 	app.mainJob = lib.NewServiceJob(app.run)
@@ -247,11 +246,10 @@ func (a *App) onWatcherEvent(ctx context.Context, event types.Event) error {
 
 	op := event.Type
 	reqID := event.Resource.GetName()
-	log := a.log.With("request_id", reqID)
+	log := a.log.With("request_id", reqID, "request_op", op.String())
 
 	switch op {
 	case types.OpPut:
-		log = log.With("request_op", "put")
 		req, ok := event.Resource.(types.AccessRequest)
 		if !ok {
 			return trace.Errorf("unexpected resource type %T", event.Resource)
@@ -282,8 +280,6 @@ func (a *App) onWatcherEvent(ctx context.Context, event types.Event) error {
 		return nil
 
 	case types.OpDelete:
-		log = log.With("request_op", "delete")
-
 		log.DebugContext(ctx, "Expiration request received")
 
 		if err := a.onDeletedRequest(ctx, reqID); err != nil {

diff --git a/integrations/access/msteams/bot.go b/integrations/access/msteams/bot.go
@@ -119,10 +119,12 @@ func (b *Bot) GetTeamsApp(ctx context.Context) (*msapi.TeamsApp, error) {
 	}
 
 	b.log.DebugContext(ctx, "Retrieved the Teams application from the organization store",
-		"teams_app_id", teamsApp.ID,
-		"teams_app_display_name", teamsApp.DisplayName,
-		"teams_app_external_id", teamsApp.ExternalID,
-		"teams_app_distribution_method", teamsApp.DistributionMethod)
+		slog.Group("teams_app",
+			"id", teamsApp.ID,
+			"display_name", teamsApp.DisplayName,
+			"external_id", teamsApp.ExternalID,
+			"distribution_method", teamsApp.DistributionMethod),
+	)
 	b.teamsApp = teamsApp
 	return b.teamsApp, nil
 }
@@ -186,12 +188,15 @@ func (b *Bot) FetchRecipient(ctx context.Context, recipient string) (*RecipientD
 	b.mu.RUnlock()
 	if ok {
 		log.DebugContext(ctx, "Found recipient in cache",
-			"recipient_id", d.ID,
-			"recipient_installed_app_id", d.App.ID,
-			"recipient_chat_id", d.Chat.ID,
-			"recipient_chat_url", d.Chat.WebURL,
-			"recipient_chat_tenant_id", d.Chat.TenantID,
-			"recipient_kind", d.Kind)
+			slog.Group("recipient",
+				"id", d.ID,
+				"installed_app_id", d.App.ID,
+				"chat_id", d.Chat.ID,
+				"chat_url", d.Chat.WebURL,
+				"chat_tenant_id", d.Chat.TenantID,
+				"kind", d.Kind,
+			),
+		)
 		return &d, nil
 	}
 
@@ -200,11 +205,14 @@ func (b *Bot) FetchRecipient(ctx context.Context, recipient string) (*RecipientD
 	channel, isChannel := b.checkChannelURL(recipient)
 	if isChannel {
 		log.DebugContext(ctx, "Recipient is a valid channel",
-			"channel_name", channel.Name,
-			"channel_chat_id", channel.ChatID,
-			"channel_group", channel.Group,
-			"channel_tenant_id", channel.Tenant,
-			"channel_url", channel.URL)
+			slog.Group("channel",
+				"name", channel.Name,
+				"chat_id", channel.ChatID,
+				"group", channel.Group,
+				"tenant_id", channel.Tenant,
+				"url", channel.URL,
+			),
+		)
 		// A team and a group are different but in MsTeams the team is associated to a group and will have the same id.
 
 		log = log.With("teams_app_id", b.teamsApp.ID, "channel_group", channel.Group)
@@ -430,10 +438,13 @@ func (b *Bot) checkChannelURL(recipient string) (*Channel, bool) {
 		ChatID: chatID,
 	}
 	log.DebugContext(ctx, "The recipient is a channel",
-		"channel_name", channel.Name,
-		"channel_group_id", channel.Group,
-		"channel_tenant_id", channel.Tenant,
-		"channel_chat_id", channel.ChatID)
+		slog.Group("channel",
+			"name", channel.Name,
+			"group_id", channel.Group,
+			"tenant_id", channel.Tenant,
+			"chat_id", channel.ChatID,
+		),
+	)
 
 	return &channel, true
 }
diff --git a/integrations/access/msteams/validate.go b/integrations/access/msteams/validate.go
@@ -142,7 +142,7 @@ func loadConfig(configPath string) (*Bot, *Config, error) {
 
 	fmt.Printf(" - Checking application %v status...\n", c.MSAPI.TeamsAppID)
 
-	log, err := c.Log.NewSLog()
+	log, err := c.Log.NewSLogLogger()
 	if err != nil {
 		return nil, nil, trace.Wrap(err)
 	}

diff --git a/integrations/lib/logger/logger.go b/integrations/lib/logger/logger.go
@@ -98,8 +98,9 @@ func Setup(conf Config) error {
 	return nil
 }
 
-// NewSLog builds a slog.Logger from the logger.Config.
-func (conf Config) NewSLog() (*slog.Logger, error) {
+// NewSLogLogger builds a slog.Logger from the logger.Config.
+// TODO: this code is adapted from `config.applyLogConfig`, we'll want to deduplicate the logic next time we refactor the logging setup
+func (conf Config) NewSLogLogger() (*slog.Logger, error) {
 	var w io.Writer
 	switch conf.Output {
 	case "":

diff --git a/lib/config/configuration.go b/lib/config/configuration.go
@@ -768,6 +768,8 @@ func applyAuthOrProxyAddress(fc *FileConfig, cfg *servicecfg.Config) error {
 }
 
 func applyLogConfig(loggerConfig Log, cfg *servicecfg.Config) error {
+	// TODO: this code is copied in the access plugin logging setup `logger.Config.NewSLogLogger`
+	// We'll want to deduplicate the logic next time we refactor the logging setup
 	logger := log.StandardLogger()
 
 	var w io.Writer