Skip to content

Commit 0de3ba3

Browse files
aphralGaphralG
authored
Ignore Access Log in metrics collection if Formats differ (#1097)
* add nil check * fix race conditions * fix race conditions * fix race conditions * fix issue when two formats are set for the same access log * clean up * format * change log * change log
1 parent befe09b commit 0de3ba3

File tree

5 files changed

+143
-9
lines changed

5 files changed

+143
-9
lines changed

‎internal/collector/otel_collector_plugin.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro
127127
return errors.New("OTel collector already running")
128128
}
129129

130+
slog.InfoContext(ctx, "Starting OTel collector")
130131
bootErr := oc.bootup(runCtx)
131132
if bootErr != nil {
132133
slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr)
@@ -161,7 +162,6 @@ func (oc *Collector) processReceivers(ctx context.Context, receivers []config.Ot
161162
}
162163

163164
func (oc *Collector) bootup(ctx context.Context) error {
164-
slog.InfoContext(ctx, "Starting OTel collector")
165165
errChan := make(chan error)
166166

167167
go func() {
@@ -266,7 +266,7 @@ func (oc *Collector) handleNginxConfigUpdate(ctx context.Context, msg *bus.Messa
266266
reloadCollector := oc.checkForNewReceivers(nginxConfigContext)
267267

268268
if reloadCollector {
269-
slog.InfoContext(ctx, "Reloading OTel collector config")
269+
slog.InfoContext(ctx, "Reloading OTel collector config, nginx config updated")
270270
err := writeCollectorConfig(oc.config.Collector)
271271
if err != nil {
272272
slog.ErrorContext(ctx, "Failed to write OTel Collector config", "error", err)
@@ -291,7 +291,7 @@ func (oc *Collector) handleResourceUpdate(ctx context.Context, msg *bus.Message)
291291
headersSetterExtensionUpdated := oc.updateHeadersSetterExtension(ctx, resourceUpdateContext)
292292

293293
if resourceProcessorUpdated || headersSetterExtensionUpdated {
294-
slog.InfoContext(ctx, "Reloading OTel collector config")
294+
slog.InfoContext(ctx, "Reloading OTel collector config, resource updated")
295295
err := writeCollectorConfig(oc.config.Collector)
296296
if err != nil {
297297
slog.ErrorContext(ctx, "Failed to write OTel Collector config", "error", err)
@@ -387,6 +387,7 @@ func (oc *Collector) restartCollector(ctx context.Context) {
387387
return
388388
}
389389

390+
slog.InfoContext(ctx, "Restarting OTel collector")
390391
bootErr := oc.bootup(runCtx)
391392
if bootErr != nil {
392393
slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr)

‎internal/command/command_plugin_test.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,6 @@ func TestCommandPlugin_monitorSubscribeChannel(t *testing.T) {
257257
}
258258

259259
func TestCommandPlugin_FeatureDisabled(t *testing.T) {
260-
// logBuf := &bytes.Buffer{}
261-
// stub.StubLoggerWith(logBuf)
262-
263260
tests := []struct {
264261
managementPlaneRequest *mpi.ManagementPlaneRequest
265262
expectedLog string

‎internal/datasource/config/nginx_config_parser.go‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
144144
if !ncp.ignoreLog(directive.Args[0]) {
145145
accessLog := ncp.accessLog(directive.Args[0], ncp.accessLogDirectiveFormat(directive),
146146
formatMap)
147-
nginxConfigContext.AccessLogs = append(nginxConfigContext.AccessLogs, accessLog)
147+
nginxConfigContext.AccessLogs = ncp.addAccessLog(accessLog, nginxConfigContext.AccessLogs)
148148
}
149149
case "error_log":
150150
if !ncp.ignoreLog(directive.Args[0]) {
@@ -214,6 +214,29 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
214214
return nginxConfigContext, nil
215215
}
216216

217+
func (ncp *NginxConfigParser) addAccessLog(accessLog *model.AccessLog,
218+
accessLogs []*model.AccessLog,
219+
) []*model.AccessLog {
220+
for i, log := range accessLogs {
221+
if accessLog.Name == log.Name {
222+
if accessLog.Format != log.Format {
223+
slog.Warn("Found multiple log_format directives for the same access log. Multiple log formats "+
224+
"are not supported in the same access log, metrics from this access log "+
225+
"will not be collected", "access_log", accessLog.Name)
226+
227+
return append(accessLogs[:i], accessLogs[i+1:]...)
228+
}
229+
slog.Debug("Found duplicate access log, skipping", "access_log", accessLog.Name)
230+
231+
return accessLogs
232+
}
233+
}
234+
235+
slog.Debug("Found valid access log", "access_log", accessLog.Name)
236+
237+
return append(accessLogs, accessLog)
238+
}
239+
217240
func (ncp *NginxConfigParser) crossplaneConfigTraverse(
218241
ctx context.Context,
219242
root *crossplane.Config,

‎internal/datasource/config/nginx_config_parser_test.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -557,6 +557,120 @@ func TestNginxConfigParser_sslCert(t *testing.T) {
557557
assert.Equal(t, certFile, sslCert.GetFileMeta().GetName())
558558
}
559559

560+
func TestNginxConfigParser_checkLog(t *testing.T) {
561+
logBuf := &bytes.Buffer{}
562+
stub.StubLoggerWith(logBuf)
563+
tests := []struct {
564+
name string
565+
expectedLog string
566+
accessLog *model.AccessLog
567+
currentAccessLogs []*model.AccessLog
568+
expectedAccessLogs []*model.AccessLog
569+
}{
570+
{
571+
name: "Test 1: valid access log",
572+
accessLog: &model.AccessLog{
573+
Name: "/var/log/nginx/access2.log",
574+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
575+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
576+
Permissions: "",
577+
Readable: true,
578+
},
579+
currentAccessLogs: []*model.AccessLog{
580+
{
581+
Name: "/var/log/nginx/access.log",
582+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
583+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
584+
Permissions: "",
585+
Readable: true,
586+
},
587+
},
588+
expectedAccessLogs: []*model.AccessLog{
589+
{
590+
Name: "/var/log/nginx/access.log",
591+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
592+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
593+
Permissions: "",
594+
Readable: true,
595+
},
596+
{
597+
Name: "/var/log/nginx/access2.log",
598+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
599+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
600+
Permissions: "",
601+
Readable: true,
602+
},
603+
},
604+
expectedLog: "Found valid access log",
605+
},
606+
{
607+
name: "Test 2: Duplicate access log, with same format",
608+
accessLog: &model.AccessLog{
609+
Name: "/var/log/nginx/access.log",
610+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
611+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
612+
Permissions: "",
613+
Readable: true,
614+
},
615+
currentAccessLogs: []*model.AccessLog{
616+
{
617+
Name: "/var/log/nginx/access.log",
618+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
619+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
620+
Permissions: "",
621+
Readable: true,
622+
},
623+
},
624+
expectedAccessLogs: []*model.AccessLog{
625+
{
626+
Name: "/var/log/nginx/access.log",
627+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
628+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
629+
Permissions: "",
630+
Readable: true,
631+
},
632+
},
633+
expectedLog: "Found duplicate access log, skipping",
634+
},
635+
636+
{
637+
name: "Test 3: invalid access log, duplicate access log with different format",
638+
accessLog: &model.AccessLog{
639+
Name: "/var/log/nginx/access.log",
640+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
641+
"\"$http_x_forwarded_for\"$status $body_bytes_sent",
642+
Permissions: "",
643+
Readable: true,
644+
},
645+
currentAccessLogs: []*model.AccessLog{
646+
{
647+
Name: "/var/log/nginx/access.log",
648+
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
649+
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
650+
Permissions: "",
651+
Readable: true,
652+
},
653+
},
654+
expectedAccessLogs: []*model.AccessLog{},
655+
expectedLog: "Found multiple log_format directives for the same access log. " +
656+
"Multiple log formats are not supported in the same access log, metrics from this access log " +
657+
"will not be collected",
658+
},
659+
}
660+
661+
for _, test := range tests {
662+
t.Run(test.name, func(t *testing.T) {
663+
ncp := NewNginxConfigParser(types.AgentConfig())
664+
logs := ncp.addAccessLog(test.accessLog, test.currentAccessLogs)
665+
assert.Equal(t, test.expectedAccessLogs, logs)
666+
667+
helpers.ValidateLog(t, test.expectedLog, logBuf)
668+
669+
logBuf.Reset()
670+
})
671+
}
672+
}
673+
560674
// nolint: maintidx
561675
func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) {
562676
tmpDir := t.TempDir()

‎internal/watcher/process/process_operator.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,8 @@ import (
99
"context"
1010
"strings"
1111

12-
"github.com/shirou/gopsutil/v4/process"
13-
1412
"github.com/nginx/agent/v3/pkg/nginxprocess"
13+
"github.com/shirou/gopsutil/v4/process"
1514
)
1615

1716
//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.8.1 -generate

0 commit comments

Comments
 (0)