Skip to content

Conversation

@cristiangreco
Copy link
Contributor

PR Description

Strip away comments from pg_stat_statements normalized sql queries, as postgres doesn't do that automatically.

While at it:

  • move some stuff from lexer.go closer to postgres component
  • keep a normalizer instance around in query_details.go
  • polish some test cases formatting

Which issue(s) this PR fixes

n.a.

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@cristiangreco cristiangreco marked this pull request as ready for review December 4, 2025 15:34
@cristiangreco cristiangreco requested a review from a team as a code owner December 4, 2025 15:34
Strip away comments from `pg_stat_statements` normalized sql queries,
as postgres doesn't do that automatically.

While at it:
- move some stuff from `lexer.go` closer to postgres component
- keep a normalizer instance around in `query_details.go`
- polish some test cases formatting
@cristiangreco cristiangreco force-pushed the cristian/dbo11y-pg-remove-comments branch from 982e208 to f3619fb Compare December 4, 2025 15:34
c.entryHandler.Chan() <- database_observability.BuildLokiEntry(
logging.LevelInfo,
OP_QUERY_ASSOCIATION,
fmt.Sprintf(`queryid="%s" querytext=%q datname="%s" engine="postgres"`, queryID, queryText, databaseName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we're removing the db engine? I understand that postgres is uniquely identified by the other values, I.E. queryid and datname. That said, having the engine listed doesn't increase cardinality, and isn't a huge increase in log line size either.

I don't have a strong opinion either way, just curious what the motivation is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that postgres is uniquely identified by the other values, I.E. queryid and datname. That said, having the engine listed doesn't increase cardinality, and isn't a huge increase in log line size either.

Yes that's correct, it doesn't really change the cardinality. It was added there at early development stage though and we've removed it from various places over time (this is likely the last place where it appears). Not a big deal, doing it just for consistency at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants