feat(outputs.timestream): Support ingesting multi-measures #11385

lohithshetty · 2022-06-24T02:31:20Z

Required for all PRs:

Updated associated README.md.
Wrote appropriate unit tests.
Pull request title or commits are in conventional commit format

Feature: support ingesting multi-measures to timestream

The feature controlled using the use_multi_measure_records field which is set to false by default for backward compatibility.

reimda · 2022-06-24T17:38:41Z

Hi Lohith, thanks for the PR. Could you update the sample config to include the new settings? https://github.com/influxdata/telegraf/blob/master/docs/developers/SAMPLE_CONFIG.md#style

Also update readme.md to describe how the settings are useful and maybe link to timestream documentation?

srebhan · 2022-07-08T13:49:02Z

Hey @lohithshetty, how does this code relate to the other two PRs (#11094 and #10952) with the exactly same PR title? Can we close those?

Updated readme to include sample config for using multi-measure records

lohithshetty · 2022-07-13T18:51:23Z

Hi reimda@, apologies for the delayed response.
I've updated readme.md to include details about multi-measure feature. Thank you.

lohithshetty · 2022-07-13T18:52:19Z

Hi @srebhan the other two PRs (#11094 and #10952) can be closed as this PR covers the same change. Thank you.

srebhan

Thanks for updateing the PR @lohithshetty! I have some minor comments in the code...

plugins/outputs/timestream/timestream.go

srebhan · 2022-07-15T09:22:17Z

plugins/outputs/timestream/timestream.go

 		if t.SingleTableDimensionNameForTelegrafMeasurementName != "" {
 			return fmt.Errorf("in '%s' mapping mode, do not specify SingleTableDimensionNameForTelegrafMeasurementName key", MappingModeMultiTable)
 		}
+
+		if t.UseMultiMeasureRecords && t.MeasureNameForMultiMeasureRecords == "" {
+			return fmt.Errorf("in '%s' mapping mode, with multi-measure enabled, key MeasureNameForMultiMeasureRecords is required", MappingModeMultiTable)
+		}


Why is this different from the code in lines 104 to 110?

@srebhan
When ingesting in MappingModeSingleTable and UseMultiMeasureRecords is enabled, we use measurementName ( from line protocol ) as multiMeasure name in timestream, where as in MappingModeMultiTable mode ( data is ingested to multiple tables ) and UseMultiMeasureRecords is enabled, we ask for measureName(MeasureNameForMultiMeasureRecords) that needs to be used, this is due to the fact that measurementName(line protocol) is used as timestream table name. Hope that clarifies, Readme has some details about different combinations for these configs.

@nirmeshk Please add any details I might have missed here.

Fair enough. It would be good if you can add this explanation as a comment to the code...

The comment is still missing.

plugins/outputs/timestream/timestream.go

srebhan · 2022-07-21T08:34:37Z

@lohithshetty I think you did not push the changes yet, did you? Let me know once you are ready for another review round...

lohithshetty · 2022-07-22T22:14:21Z

@srebhan I pushed the changes, can you please take a look. Thank you.

srebhan · 2022-07-26T15:59:20Z

Hey @lohithshetty thanks for the push. This leaves us with the question on why lines 123 - 126 are different in structure... Can you please comment on this?!

lohithshetty · 2022-08-11T16:45:17Z

Hi @srebhan, would appreciate your help in reviewing and closing this PR. Thank you.

srebhan · 2022-08-16T09:47:13Z

@lohithshetty beside my comment, can you please run make docs and push the changes to fix the unit tests!?

plugins/outputs/timestream/README.md

Update sample.conf to include new documentation, disabled failing test ( will re-visit with a follow up)

lohithshetty · 2022-08-24T22:07:42Z

Hi @srebhan, Thanks for reviewing the pull request, I've made the changes as per your suggestion. Currently circle-ci failing for mac for plugin plugins/outputs/graylog ( not related to this pull request ). Appreciate merging this changes. Thank you.

kingston88 · 2022-09-08T16:35:55Z

@srebhan Are we good to merge this change this week?

plugins/outputs/timestream/sample.conf

moved lengthy documentation from toml to README.md

Linting errors

kingston88 · 2022-09-09T15:35:10Z

@reimda @srebhan Can we merge this change?

reimda · 2022-09-09T16:29:23Z

@reimda @srebhan Can we merge this change?

We merge after two review approvals, so it's close. It looks like there is one outstanding item from srebhan's review that needs to be taken care of. Just adding some comments?

lohithshetty · 2022-09-13T17:16:29Z

@reimda @srebhan
README has a section which explains different modes with examples ( refer to Mapping data from Influx to Timestream section in README). Please let me know if you think more explanation/comments are required.

srebhan

Thanks @lohithshetty for the update! There are some small things here and there, but only formatting mostly. Can you please care for the comment documenting why the structure differs?

plugins/outputs/timestream/README.md

srebhan · 2022-09-16T16:30:44Z

plugins/outputs/timestream/timestream.go

 		if t.SingleTableDimensionNameForTelegrafMeasurementName != "" {
 			return fmt.Errorf("in '%s' mapping mode, do not specify SingleTableDimensionNameForTelegrafMeasurementName key", MappingModeMultiTable)
 		}
+
+		if t.UseMultiMeasureRecords && t.MeasureNameForMultiMeasureRecords == "" {
+			return fmt.Errorf("in '%s' mapping mode, with multi-measure enabled, key MeasureNameForMultiMeasureRecords is required", MappingModeMultiTable)
+		}


The comment is still missing.

plugins/outputs/timestream/timestream.go

plugins/outputs/timestream/timestream_test.go

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

linting

telegraf-tiger · 2022-09-16T21:15:26Z

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

🥳 This pull request decreases the Telegraf binary size by -1.60 % for linux amd64 (new size: 150.2 MB, nightly size 152.6 MB)

📦 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_i386.zip
armel.deb	armv6hl.rpm	freebsd_amd64.tar.gz
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
		static_linux_amd64.tar.gz

srebhan

Looks good to me. Thanks for your contribution @lohithshetty!

…a#11385)

hadiel05 · 2022-11-03T18:26:34Z

Is this released? tested with this tag telegraf:1.24.3 and seems like its not.

powersj · 2022-11-03T18:39:38Z

Anything tagged a new "feature" is released in the next minor version. This should go out in v1.25.0

feat: support ingesting multi-measures to timestream

e65abbb

telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 24, 2022

lohithshetty mentioned this pull request Jun 24, 2022

feat: support ingesting multi-measures to timestream #11384

Closed

feat: support ingesting multi-measures to timestream

60b746a

This was referenced Jul 8, 2022

feat: support ingesting multi-measures to timestream #11094

Closed

feat: support ingesting multi-measures to timestream #10952

Closed

srebhan self-assigned this Jul 8, 2022

lohithshetty added 3 commits July 13, 2022 11:44

Merge branch 'influxdata:master' into master

8008ea7

Update README.md

d8895c7

Updated readme to include sample config for using multi-measure records

Merge branch 'master' of https://github.com/lohithshetty/telegraf

e88ff4b

srebhan reviewed Jul 15, 2022

View reviewed changes

lohithshetty added 2 commits July 20, 2022 10:44

Merge branch 'influxdata:master' into master

824caa7

Update timestream.go

95666e5

Updated after review

878d22c

powersj mentioned this pull request Aug 5, 2022

timestream output plugin with multi-measure records #10493

Closed

Merge branch 'master' into master

f41a5d7

srebhan reviewed Aug 16, 2022

View reviewed changes

plugins/outputs/timestream/README.md Show resolved Hide resolved

Update sample.conf

be08319

Update sample.conf to include new documentation, disabled failing test ( will re-visit with a follow up)

reimda reviewed Sep 8, 2022

View reviewed changes

plugins/outputs/timestream/sample.conf Show resolved Hide resolved

lohithshetty added 4 commits September 8, 2022 16:37

Updated README to include data mapping

68b2dad

moved lengthy documentation from toml to README.md

Update README.md

e0362a7

Update README.md

4267933

Linting errors

Update README

926ae6b

reimda approved these changes Sep 9, 2022

View reviewed changes

srebhan reviewed Sep 16, 2022

View reviewed changes

lohithshetty and others added 10 commits September 16, 2022 12:26

Update plugins/outputs/timestream/timestream_test.go

1b223f7

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/outputs/timestream/timestream_test.go

cd54693

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/outputs/timestream/README.md

1295ed8

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Update plugins/outputs/timestream/README.md

417093a

Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

Updated to address review comments

dc585ca

Update README

813173e

linting

Update README

1a4a002

linting

Update README

d91e5b9

linting

Update README

c32a8b0

Update README

3c40589

linting

srebhan approved these changes Sep 19, 2022

View reviewed changes

srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. plugin/output 1. Request for new output plugins 2. Issues/PRs that are related to out plugins labels Sep 19, 2022

srebhan changed the title ~~feat: support ingesting multi-measures to timestream~~ feat(outputs.timestream): Support ingesting multi-measures Sep 19, 2022

srebhan merged commit 8dd7ec0 into influxdata:master Sep 20, 2022

dba-leshop pushed a commit to dba-leshop/telegraf that referenced this pull request Oct 30, 2022

feat(outputs.timestream): Support ingesting multi-measures (influxdat…

d91fbd3

…a#11385)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(outputs.timestream): Support ingesting multi-measures #11385

feat(outputs.timestream): Support ingesting multi-measures #11385

lohithshetty commented Jun 24, 2022 •

edited by powersj

reimda commented Jun 24, 2022

srebhan commented Jul 8, 2022

lohithshetty commented Jul 13, 2022 •

edited

lohithshetty commented Jul 13, 2022 •

edited

srebhan left a comment

srebhan Jul 15, 2022

lohithshetty Jul 27, 2022 •

edited

lohithshetty Jul 27, 2022

srebhan Aug 16, 2022

srebhan Sep 16, 2022

srebhan commented Jul 21, 2022

lohithshetty commented Jul 22, 2022

srebhan commented Jul 26, 2022

lohithshetty commented Aug 11, 2022

srebhan commented Aug 16, 2022

lohithshetty commented Aug 24, 2022

kingston88 commented Sep 8, 2022

kingston88 commented Sep 9, 2022

reimda commented Sep 9, 2022

lohithshetty commented Sep 13, 2022

srebhan left a comment

srebhan Sep 16, 2022

telegraf-tiger bot commented Sep 16, 2022

Artifact URLs

srebhan left a comment

hadiel05 commented Nov 3, 2022

powersj commented Nov 3, 2022

feat(outputs.timestream): Support ingesting multi-measures #11385

feat(outputs.timestream): Support ingesting multi-measures #11385

Conversation

lohithshetty commented Jun 24, 2022 • edited by powersj

Required for all PRs:

reimda commented Jun 24, 2022

srebhan commented Jul 8, 2022

lohithshetty commented Jul 13, 2022 • edited

lohithshetty commented Jul 13, 2022 • edited

srebhan left a comment

Choose a reason for hiding this comment

srebhan Jul 15, 2022

Choose a reason for hiding this comment

lohithshetty Jul 27, 2022 • edited

Choose a reason for hiding this comment

lohithshetty Jul 27, 2022

Choose a reason for hiding this comment

srebhan Aug 16, 2022

Choose a reason for hiding this comment

srebhan Sep 16, 2022

Choose a reason for hiding this comment

srebhan commented Jul 21, 2022

lohithshetty commented Jul 22, 2022

srebhan commented Jul 26, 2022

lohithshetty commented Aug 11, 2022

srebhan commented Aug 16, 2022

lohithshetty commented Aug 24, 2022

kingston88 commented Sep 8, 2022

kingston88 commented Sep 9, 2022

reimda commented Sep 9, 2022

lohithshetty commented Sep 13, 2022

srebhan left a comment

Choose a reason for hiding this comment

srebhan Sep 16, 2022

Choose a reason for hiding this comment

telegraf-tiger bot commented Sep 16, 2022

Artifact URLs

srebhan left a comment

Choose a reason for hiding this comment

hadiel05 commented Nov 3, 2022

powersj commented Nov 3, 2022

lohithshetty commented Jun 24, 2022 •

edited by powersj

lohithshetty commented Jul 13, 2022 •

edited

lohithshetty commented Jul 13, 2022 •

edited

lohithshetty Jul 27, 2022 •

edited