Skip to content
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.

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

Tracelog uses "time" when logging which can cause issues with JSON payloads #2043

Open
vicentsegui opened this issue Jun 13, 2024 · 2 comments
Labels

Comments

@vicentsegui
Copy link

vicentsegui commented Jun 13, 2024

Describe the bug
The tracelog packlage (github.com/jackc/pgx/v5/tracelog") uses "time" as a label to print the time used to process. This however causes conflicts with other logging systems that will use "time" as a label to mean timestamp of log produced. This causes log ingestors to fail parsing the JSON as there is a duplicate key.

To Reproduce
Steps to reproduce the behavior:

Use github.com/jackc/pgx/v5/tracelog and github.com/jackc/pgx-zerolog and a logger configured roughly like so:

	log = zerolog.New(output).
			Level(zerolog.Level(logLevel)).
			With().
			Caller().
			Stack().
			Timestamp().
			Str("scm_version", version.ScmVersion).
			Logger()

and configure pgx pool like so:

	pgLogTrace := &tracelog.TraceLog{
		Logger:   zerologadapter.NewLogger(log),
		LogLevel: tracelog.LogLevelTrace,
	}
	pgConfig.ConnConfig.Tracer = pgxtrace.CompositeQueryTracer{
		pgLogTrace,
	}
	l.Info().Msg("Connecting to Postgres database")
	pgClient, err := pgxpool.NewWithConfig(ctx, pgConfig)

Use the pgClient to run some queries and observe the logs:

{"level":"info",
 "module":"pgx",
"alreadyPrepared":false,
"name":"stmtcache_11578b0b996ce09b82a573adef17a56ff464aab9bbc4a4af",
"pid":426317,
"sql":"SELECT *********", 
"caller":"external/gazelle~~go_deps~com_github_jackc_pgx_zerolog/adapter.go:119", 
"time":3.17694, 
"time":"2024-06-13T15:20:12.937558654Z",
"message":"Prepare",
....}

Notice the duplicate "time" entry,.

Expected behavior

The tracelog package should avoid using generic labels like "time", etc as those can likely collide with other labels. I'd suggest adding a prefix (perhaps configurable) so that it will be logged like "pgx.QueryTime", for all tracelog labels.

Version

  • Go: $ go version -> 1.22
  • pgx: github.com/jackc/pgx/v5 v5.6.0
  • pgx-zerolog: github.com/jackc/pgx-zerolog v0.0.0-20230315001418-f978528409eb
@jackc
Copy link
Owner

jackc commented Jun 13, 2024

I don't think I want to change the default name. It's been called "time" for as long as there has been logging in pgx. But I'm open to making the name of the time/duration field configurable. Presumably an extra field on tracelog.TraceLog should do it. I don't think any other fields should collide.

@vicentsegui
Copy link
Author

Sounds good to me. There is another alternative which is to change the zerologadapter (I've done that to test as a quick fix) and prepend the prefix of choice in there:

func (pl *Logger) Log(ctx context.Context, level tracelog.LogLevel, msg string, inputData map[string]interface{}) {
	var zlevel zerolog.Level
	data := make(map[string]interface{})
	for k, v := range inputData {
		data["pgx."+k] = v
	}
	switch level {
	case tracelog.LogLevelNone:
		zlevel = zerolog.NoLevel
	case tracelog.LogLevelError:
		zlevel = zerolog.ErrorLevel
	case tracelog.LogLevelWarn:
		zlevel = zerolog.WarnLevel
	case tracelog.LogLevelInfo:
		zlevel = zerolog.InfoLevel
	case tracelog.LogLevelDebug:
		zlevel = zerolog.DebugLevel
	case tracelog.LogLevelTrace:
		zlevel = zerolog.TraceLevel
	default:
		zlevel = zerolog.DebugLevel
	}

	var zctx zerolog.Context
	if pl.fromContext {
		logger := zerolog.Ctx(ctx)
		zctx = logger.With()
	} else {
		zctx = pl.logger.With()
	}
	if pl.withFunc != nil {
		zctx = pl.withFunc(ctx, zctx)
	}

	pgxlog := zctx.Logger()
	event := pgxlog.WithLevel(zlevel)
	if event.Enabled() {
		if pl.fromContext && !pl.skipModule {
			event.Str("module", "pgx")
		}

		if pl.subDictionary {
			event.Dict(pl.subDictionaryKey, zerolog.Dict().Fields(data))
		} else {
			event.Fields(data)
		}

		event.Msg(msg)
	}
}

However this creates a copy and more allocations so I think simply changing the tracelog package is for the best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants