Conversation
…o facilitate better testing
…ort more possible/probable version strings
rgeyer
changed the title
rgeyer
changed the title
| running: atomic.NewBool(false), | ||
| }, nil | ||
| } | ||
| // Pre-sanitize the version a bit before semver gets it |
There was a problem hiding this comment.
nit: "a bit" is not very clear
There was a problem hiding this comment.
Fair enough, hopefully it is more informative now.
There was a problem hiding this comment.
Should we add a test for the exact versions that are failing today?
There was a problem hiding this comment.
Did do, on lines 2293-2305. That said, this is just one of the failing version strings, but it's one I directly experienced with the postgres docker image.
I didn't get a chance to grab the full Percona version number that's failing, but the error is the same, semver is trying to parse spaces and special characters as a minor version. The changes (and the test) account for basically any combination of that.
cristiangreco
left a comment
cristiangreco
left a comment
There was a problem hiding this comment.
Approving as I see Matt's comments have been addressed already
…observability component (#5131) * Move semver parsing of postgres version into explain plan collector to facilitate better testing * More guardrails for parsing the postgres versions with semver to support more possible/probable version strings * More useful comments * Add percona version string test
…observability component (#5131) * Move semver parsing of postgres version into explain plan collector to facilitate better testing * More guardrails for parsing the postgres versions with semver to support more possible/probable version strings * More useful comments * Add percona version string test
…observability component (#5131) * Move semver parsing of postgres version into explain plan collector to facilitate better testing * More guardrails for parsing the postgres versions with semver to support more possible/probable version strings * More useful comments * Add percona version string test
3 participants
PR Description
Effectively any version string that wasn't a very traditional semver string was failing to parse. This would impact many installations but was called out specifically for a Percona DB and a postgres docker image.
Specifically, these strings had "extra" bits at the end of the version string which semver was treating as part of the minor version.
PR Checklist
BEGIN_COMMIT_OVERRIDE
fix(database_observability): Improve postgres version parsing for explain plans in database_observability component (#5131)
END_COMMIT_OVERRIDE