Skip to content

Ignore Access Log in metrics collection if Formats differ #1097

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
May 30, 2025
Merged

Ignore Access Log in metrics collection if Formats differ #1097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2f423f1
add nil check
aphralG May 27, 2025
e07ebbb
fix race conditions
aphralG May 28, 2025
cab1df1
fix race conditions
aphralG May 28, 2025
846c064
fix race conditions
aphralG May 28, 2025
2c04c8e
fix issue when two formats are set for the same access log
aphralG May 29, 2025
660afbd
clean up
aphralG May 29, 2025
b017233
format
aphralG May 29, 2025
b982a31
change log
aphralG May 29, 2025
fa763de
change log
aphralG May 29, 2025
c83e0b9
Merge branch 'v3' into fix-panic-with-two-log-formats
aphralG May 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions internal/collector/otel_collector_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func (oc *Collector) Init(ctx context.Context, mp bus.MessagePipeInterface) erro
return errors.New("OTel collector already running")
}

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

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

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

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

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

slog.InfoContext(ctx, "Restarting OTel collector")
bootErr := oc.bootup(runCtx)
if bootErr != nil {
slog.ErrorContext(runCtx, "Unable to start OTel Collector", "error", bootErr)
Expand Down
3 changes: 0 additions & 3 deletions internal/command/command_plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,6 @@ func TestCommandPlugin_monitorSubscribeChannel(t *testing.T) {
}

func TestCommandPlugin_FeatureDisabled(t *testing.T) {
// logBuf := &bytes.Buffer{}
// stub.StubLoggerWith(logBuf)

tests := []struct {
managementPlaneRequest *mpi.ManagementPlaneRequest
expectedLog string
Expand Down
25 changes: 24 additions & 1 deletion internal/datasource/config/nginx_config_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
if !ncp.ignoreLog(directive.Args[0]) {
accessLog := ncp.accessLog(directive.Args[0], ncp.accessLogDirectiveFormat(directive),
formatMap)
nginxConfigContext.AccessLogs = append(nginxConfigContext.AccessLogs, accessLog)
nginxConfigContext.AccessLogs = ncp.addAccessLog(accessLog, nginxConfigContext.AccessLogs)
}
case "error_log":
if !ncp.ignoreLog(directive.Args[0]) {
Expand Down Expand Up @@ -214,6 +214,29 @@ func (ncp *NginxConfigParser) createNginxConfigContext(
return nginxConfigContext, nil
}

func (ncp *NginxConfigParser) addAccessLog(accessLog *model.AccessLog,
accessLogs []*model.AccessLog,
) []*model.AccessLog {
for i, log := range accessLogs {
if accessLog.Name == log.Name {
if accessLog.Format != log.Format {
slog.Warn("Found multiple log_format directives for the same access log. Multiple log formats "+
"are not supported in the same access log, metrics from this access log "+
"will not be collected", "access_log", accessLog.Name)

return append(accessLogs[:i], accessLogs[i+1:]...)
}
slog.Debug("Found duplicate access log, skipping", "access_log", accessLog.Name)

return accessLogs
}
}

slog.Debug("Found valid access log", "access_log", accessLog.Name)

return append(accessLogs, accessLog)
}

func (ncp *NginxConfigParser) crossplaneConfigTraverse(
ctx context.Context,
root *crossplane.Config,
Expand Down
114 changes: 114 additions & 0 deletions internal/datasource/config/nginx_config_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,120 @@ func TestNginxConfigParser_sslCert(t *testing.T) {
assert.Equal(t, certFile, sslCert.GetFileMeta().GetName())
}

func TestNginxConfigParser_checkLog(t *testing.T) {
logBuf := &bytes.Buffer{}
stub.StubLoggerWith(logBuf)
tests := []struct {
name string
expectedLog string
accessLog *model.AccessLog
currentAccessLogs []*model.AccessLog
expectedAccessLogs []*model.AccessLog
}{
{
name: "Test 1: valid access log",
accessLog: &model.AccessLog{
Name: "/var/log/nginx/access2.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
currentAccessLogs: []*model.AccessLog{
{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
},
expectedAccessLogs: []*model.AccessLog{
{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
{
Name: "/var/log/nginx/access2.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
},
expectedLog: "Found valid access log",
},
{
name: "Test 2: Duplicate access log, with same format",
accessLog: &model.AccessLog{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
currentAccessLogs: []*model.AccessLog{
{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
},
expectedAccessLogs: []*model.AccessLog{
{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
},
expectedLog: "Found duplicate access log, skipping",
},

{
name: "Test 3: invalid access log, duplicate access log with different format",
accessLog: &model.AccessLog{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent",
Permissions: "",
Readable: true,
},
currentAccessLogs: []*model.AccessLog{
{
Name: "/var/log/nginx/access.log",
Format: "$remote_addr - $remote_user [$time_local] \"$request\" \"$http_user_agent\" " +
"\"$http_x_forwarded_for\"$status $body_bytes_sent \"$http_referer\"",
Permissions: "",
Readable: true,
},
},
expectedAccessLogs: []*model.AccessLog{},
expectedLog: "Found multiple log_format directives for the same access log. " +
"Multiple log formats are not supported in the same access log, metrics from this access log " +
"will not be collected",
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
ncp := NewNginxConfigParser(types.AgentConfig())
logs := ncp.addAccessLog(test.accessLog, test.currentAccessLogs)
assert.Equal(t, test.expectedAccessLogs, logs)

helpers.ValidateLog(t, test.expectedLog, logBuf)

logBuf.Reset()
})
}
}

// nolint: maintidx
func TestNginxConfigParser_urlsForLocationDirective(t *testing.T) {
tmpDir := t.TempDir()
Expand Down
3 changes: 1 addition & 2 deletions internal/watcher/process/process_operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ import (
"context"
"strings"

"github.com/shirou/gopsutil/v4/process"

"github.com/nginx/agent/v3/pkg/nginxprocess"
"github.com/shirou/gopsutil/v4/process"
)

//go:generate go run github.com/maxbrunsfeld/counterfeiter/[email protected] -generate
Expand Down