feat(outputs.sql): Add option to automate table schema updates #16544

hautecodure · 2025-02-24T12:54:53Z

Automate table updates, when the table does not contain a field

Summary

Currently, updates to the Table are left to the administrator and whenever a field is not present (in the table), the entire Telegraf flush is discarded, losing data in the process.

The new approach exposes two templates, one to update a table and another on to query a table for it's columns.

To avoid unnecessary (and expensive) requests, we cache the table columns and update it, whenever there is a cache miss from a field.

Checklist

No AI generated code was used in this PR

Related issues

N/A

telegraf-tiger · 2025-02-24T12:55:02Z

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

hautecodure · 2025-02-24T17:28:20Z

Pushed a new version to address the linter complaints and took the opportunity to:

Get the defer outside of the for loop
Move the table creation in a separate function to make the Write loop to improve readability

Note: left the table_exists_template option, even though nothing uses it, as removing it might break existing configs

Update:

It seems the Integration tests failing, are addressed in another PR. Will rebase, once merged.

srebhan

@hautecodure thank you very much for your contribution! Much appreciated! I do have some comments in the code, the biggest work items are

Strip out the column sorting to another PR to only have one thing in each PR!
I would rather like to have table-altering as an opt-in to not accidentally break existing configurations.

Looking forward to the next round!

plugins/outputs/sql/sample.conf

plugins/outputs/sql/README.md

plugins/outputs/sql/sql.go

hautecodure · 2025-02-27T19:07:41Z

Pushed a new update:

Added tests
Reworked the table query, as it did not work under Clickhouse.
The column query has been removed
Changed a few if/else expression to match the style convention

While a bit unpleasant with the per-driver overrides, there was not much of a choice, unless we want to pester users with writing queries for each and every edge-case.

srebhan

Thanks @hautecodure for the update and your patience! I do have some styling comments in the code. Additionally, I have the following items:

Please do not remove existing options (table_exists_template) or render them non-functional! This might break some users! I agree that most likely this option is useless and should be removed, but to do so, you need to deprecate it first and keep it for a certain time.
The default value of table_column_template is empty and therefore, the line in the sample.conf file must reflect this! You can add a line

  ## Use the following setting for automatically adding columns:
  ## table_update_template = "ALTER TABLE {TABLE} ADD COLUMN {COLUMN}"

Using tables map[string]map[string]bool might simplify the code and improve lookups. What do you think?

plugins/outputs/sql/sql.go

plugins/outputs/sql/sqlite_test.go

plugins/outputs/sql/README.md

Automate table schema updates, when the table does not contain a field (or tag) by using template, provided by the user. Note: this feature is opt-in, it requires the user to explicitly modify their configuration files.

hautecodure · 2025-03-04T08:11:55Z

Hi, thanks for the feedback, pushed a new iteration:

Reverted tableExists back to its current form - not in the scope for this PR, might open a new one, to discuss / update (if needed).
Update the SQL struct names, to closely match the config options
Switch table cache to map[string]map[string]bool

telegraf-tiger · 2025-03-04T08:34:09Z

Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip.
Downloads for additional architectures and packages are available below.

☺️ This pull request doesn't significantly change the Telegraf binary size (less than 1%)

📦 Click here to get additional PR build artifacts

Artifact URLs

DEB	RPM	TAR GZ	ZIP
amd64.deb	aarch64.rpm	darwin_amd64.tar.gz	windows_amd64.zip
arm64.deb	armel.rpm	darwin_arm64.tar.gz	windows_arm64.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz	windows_i386.zip
armhf.deb	i386.rpm	freebsd_armv7.tar.gz
i386.deb	ppc64le.rpm	freebsd_i386.tar.gz
mips.deb	riscv64.rpm	linux_amd64.tar.gz
mipsel.deb	s390x.rpm	linux_arm64.tar.gz
ppc64el.deb	x86_64.rpm	linux_armel.tar.gz
riscv64.deb		linux_armhf.tar.gz
s390x.deb		linux_i386.tar.gz
		linux_mips.tar.gz
		linux_mipsel.tar.gz
		linux_ppc64le.tar.gz
		linux_riscv64.tar.gz
		linux_s390x.tar.gz

srebhan

Thanks @hautecodure. Some more cosmetic things like moving the conditions to the caller side so that the reader directly sees that the create/update functions are only called on purpose or opt-in.

srebhan · 2025-03-04T11:33:14Z

plugins/outputs/sql/sql.go

-		if !p.tables[tablename] && !p.tableExists(tablename) {
-			createStmt := p.generateCreateTable(metric)
-			_, err := p.db.Exec(createStmt)
-			if err != nil {
-				return err
-			}
+		if err := p.createTable(metric); err != nil {
+			return err


To be honest, I would like to keep the condition here as otherwise this reads like "always create the table"!

I was trying to keep the Write loop easier to read and handle the minutia in the respective function.

Should i revert back to the previous statement?

plugins/outputs/sql/sql.go

srebhan · 2025-03-04T11:35:30Z

plugins/outputs/sql/sql_test.go

@@ -288,7 +337,6 @@ func TestPostgresIntegration(t *testing.T) {
 	require.NoError(t, p.Connect())
 	defer p.Close()
 	require.NoError(t, p.Write(testMetrics))
-	require.NoError(t, p.Close())


Why do you remove this? This line makes sure all metrics are written...

It closes the connection and i want to re-use the existing connection, instead of setting up a new one.

The tests seem to pass fine. Are you sure Close is required to flush the metrics? Is this a pgx quirk?

srebhan · 2025-03-04T11:35:53Z

plugins/outputs/sql/sql_test.go

+
+	p.TableUpdateTemplate = "ALTER TABLE {TABLE} ADD COLUMN {COLUMN}"
+
+	require.NoError(t, p.Write(postCreateMetrics))
+
+	fields := []string{
+		"tag_add_after_create text",
+		"bool_add_after_create boolean",
+	}
+	for _, column := range fields {
+		require.Eventually(t, func() bool {
+			rc, out, err := container.Exec([]string{
+				"bash",
+				"-c",
+				"pg_dump" +
+					" --username=" + username +
+					" --no-comments" +
+					" " + dbname +
+					// pg_dump's output has comments that include build info
+					// of postgres and pg_dump. The build info changes with
+					// each release. To prevent these changes from causing the
+					// test to fail, we strip out comments. Also strip out
+					// blank lines.
+					"|grep -E -v '(^--|^$|^SET )'",
+			})
+			require.NoError(t, err)
+			require.Equal(t, 0, rc)
+
+			b, err := io.ReadAll(out)
+			require.NoError(t, err)
+
+			return bytes.Contains(b, []byte(column))
+		}, 5*time.Second, 500*time.Millisecond, column)
+	}


Please create a new testcase for this or directly set the update template! Furthermore, a comment would be nice saying something like

// Write a metric that targets the same table but has additional fields to test the automatic // column update functionality. require.NoError(t, p.Write(postCreateMetrics)) fields := []string{ "tag_add_after_create text", "bool_add_after_create boolean", } for _, column := range fields {

Same for the other spots below.

I was thinking of creating new cases, but i have to duplicate the existing ones as i have to create a table, insert metrics, add new fields.

I'm not familiar with the test infrastructure, is there a way to keep the containers running so i don't have to re-create the database?

Or should i split the cases into separate functions and re-use them?

telegraf-tiger bot added area/sql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Feb 24, 2025

hautecodure force-pushed the master branch 3 times, most recently from 30b5ab0 to ae1da3c Compare February 24, 2025 17:21

srebhan requested changes Feb 25, 2025

View reviewed changes

hautecodure closed this Feb 25, 2025

hautecodure force-pushed the master branch from ae1da3c to edb28de Compare February 25, 2025 12:31

hautecodure reopened this Feb 25, 2025

hautecodure changed the title ~~feat(outputs.sql): Add column (if missing), use Prepared statements~~ feat(outputs.sql): Add column (if missing) from the database Feb 25, 2025

hautecodure force-pushed the master branch from 100b0bc to ede92ae Compare February 25, 2025 13:06

hautecodure requested a review from srebhan February 25, 2025 13:34

srebhan reviewed Feb 25, 2025

View reviewed changes

plugins/outputs/sql/sql.go Outdated Show resolved Hide resolved

hautecodure closed this Feb 27, 2025

hautecodure force-pushed the master branch from ede92ae to 1729806 Compare February 27, 2025 16:06

hautecodure reopened this Feb 27, 2025

hautecodure requested a review from srebhan February 27, 2025 19:13

srebhan reviewed Mar 3, 2025

View reviewed changes

srebhan self-assigned this Mar 3, 2025

hautecodure closed this Mar 4, 2025

hautecodure force-pushed the master branch from 8d93d10 to 9df54ae Compare March 4, 2025 07:34

feat(outputs.sql): Add option to automate table schema updates

9f5d273

Automate table schema updates, when the table does not contain a field (or tag) by using template, provided by the user. Note: this feature is opt-in, it requires the user to explicitly modify their configuration files.

hautecodure reopened this Mar 4, 2025

hautecodure requested a review from srebhan March 4, 2025 08:12

hautecodure changed the title ~~feat(outputs.sql): Add column (if missing) from the database~~ feat(outputs.sql): Add option to automate table schema updates Mar 4, 2025

srebhan reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(outputs.sql): Add option to automate table schema updates #16544

feat(outputs.sql): Add option to automate table schema updates #16544

hautecodure commented Feb 24, 2025 •

edited

Loading

telegraf-tiger bot commented Feb 24, 2025

hautecodure commented Feb 24, 2025 •

edited

Loading

srebhan left a comment

hautecodure commented Feb 27, 2025

srebhan left a comment

hautecodure commented Mar 4, 2025

telegraf-tiger bot commented Mar 4, 2025

Artifact URLs

srebhan left a comment

srebhan Mar 4, 2025

hautecodure Mar 4, 2025 •

edited

Loading

srebhan Mar 4, 2025

hautecodure Mar 4, 2025

srebhan Mar 4, 2025

hautecodure Mar 4, 2025

feat(outputs.sql): Add option to automate table schema updates #16544

Are you sure you want to change the base?

feat(outputs.sql): Add option to automate table schema updates #16544

Conversation

hautecodure commented Feb 24, 2025 • edited Loading

Summary

Checklist

Related issues

telegraf-tiger bot commented Feb 24, 2025

hautecodure commented Feb 24, 2025 • edited Loading

srebhan left a comment

Choose a reason for hiding this comment

hautecodure commented Feb 27, 2025

srebhan left a comment

Choose a reason for hiding this comment

hautecodure commented Mar 4, 2025

telegraf-tiger bot commented Mar 4, 2025

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

srebhan Mar 4, 2025

Choose a reason for hiding this comment

hautecodure Mar 4, 2025 • edited Loading

Choose a reason for hiding this comment

srebhan Mar 4, 2025

Choose a reason for hiding this comment

hautecodure Mar 4, 2025

Choose a reason for hiding this comment

srebhan Mar 4, 2025

Choose a reason for hiding this comment

hautecodure Mar 4, 2025

Choose a reason for hiding this comment

hautecodure commented Feb 24, 2025 •

edited

Loading

hautecodure commented Feb 24, 2025 •

edited

Loading

hautecodure Mar 4, 2025 •

edited

Loading