pgx icon indicating copy to clipboard operation
pgx copied to clipboard

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

Open vicentsegui opened this issue 1 year ago • 2 comments

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

vicentsegui avatar Jun 13 '24 17:06 vicentsegui

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.

jackc avatar Jun 13 '24 23:06 jackc

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.

vicentsegui avatar Jun 14 '24 07:06 vicentsegui