fix: Add support for opcua datetime values #12101

vsinha · 2022-10-25T18:03:03Z

Resolves #12006

This PR adds support for returning OPCUA DateTime values via the opc-ua input plugin. Prior to this PR, we silently drop those values. Reopened because I made a mistake in #12020.

Details

The gopcua library returns these values as the time.Time type, which telegraf doesn't handle in metric.go:convertField. To make the smallest change possible, we simply convert the datetime value to a string in the default golang format before returning the updated value.

Example

Example telegraf.toml

[inputs.opcua]
  # Authentication details redacted
  nodes = [
  {name="TimeEnded", namespace="4", identifier_type="s", identifier="TimeEnded"},
  ]

Before:

Notice the TimeEnded value has Quality=OK but the time value itself has failed to parse or be returned. This is because it falls into the default case in metric.go:convertField

❯ go run ./cmd/telegraf --config ~/telegraf-config-file.toml --test
...
2022-10-14T17:42:28Z D! [agent] Starting service inputs
> MACHINE123,host=HOST123,id=ns\=4;s\=TimeEnded Quality="OK (0x0)" 1665769360000000000
2022-10-14T17:42:40Z D! [agent] Stopping service inputs
2022-10-14T17:42:40Z D! [agent] Input channel closed
2022-10-14T17:42:40Z D! [agent] Stopped Successfully

After:

We return the time as a string as intended

❯ go run ./cmd/telegraf --config ~/telegraf-config-file.toml --test
...
2022-10-14T17:46:49Z D! [agent] Starting service inputs
> MACHINE123,host=HOST123,id=ns\=4;s\=TimeEnded "2022-10-18 02:13:29.222 +0000 UTC",Quality="OK (0x0)" 1665769610000000000
2022-10-14T17:46:50Z D! [agent] Stopping service inputs
2022-10-14T17:46:50Z D! [agent] Input channel closed
2022-10-14T17:46:50Z D! [agent] Stopped Successfully

vsinha · 2022-10-25T18:39:21Z

the windows test failed for a seemingly unrelated reason -- perhaps a flaky test?

=== FAIL: plugins/inputs/snmp TestGetSNMPConnectionTCP (unknown)
listen tcp 127.0.0.1:56789: bind: An attempt was made to access a socket in a way forbidden by its access permissions.panic: runtime error: invalid memory address or nil pointer dereference
	panic: sync: negative WaitGroup counter
[signal 0xc0000005 code=0x0 addr=0x0 pc=0xbed5b7]

goroutine 78 [running]:
sync.(*WaitGroup).Add(0xc0003a4dd0, 0xffffffffffffffff)
	C:/Program Files/Go/src/sync/waitgroup.go:83 +0x205
sync.(*WaitGroup).Done(0x0?)
	C:/Program Files/Go/src/sync/waitgroup.go:108 +0x36
panic({0xc646c0, 0x11fd670})
	C:/Program Files/Go/src/runtime/panic.go:890 +0x262
github.com/influxdata/telegraf/plugins/inputs/snmp.stubTCPServer(0xc0003a4dd0)
	C:/Users/circleci.PACKER-633B1A5A/project/plugins/inputs/snmp/snmp_test.go:327 +0x277
created by github.com/influxdata/telegraf/plugins/inputs/snmp.TestGetSNMPConnectionTCP
	C:/Users/circleci.PACKER-633B1A5A/project/plugins/inputs/snmp/snmp_test.go:292 +0xdb

MyaLongmire · 2022-10-25T18:42:09Z

@vsinha maybe flaky, I have retriggered the test

vsinha · 2022-10-25T20:52:14Z

@LarsStegman do you know where i should leave a test to make sure DateTimes serialize properly? The test as specified here is failing

LarsStegman · 2022-10-25T21:34:52Z

Are you sure the NodeId is correct? Maybe the identifier type should be i instead of s, since the identifier is an integer.

LarsStegman · 2022-10-26T09:11:55Z

That fixed the opcua tests, you can apply the same fix for the opcua_listener tests.

MyaLongmire · 2022-10-26T09:40:01Z

@LarsStegman Thank you for your work and continued help with the opcua plugins!

vsinha · 2022-10-27T00:06:55Z

@MyaLongmire all the tests are passing except the readme linter, where this PR doesn't contain any changes to any readmes. Is this something we can sidestep?

MyaLongmire · 2022-10-27T09:14:40Z

@vsinha Yes we can sidestep it as it is failing on things you did not change :)

MyaLongmire

One other person from the team will need to also approve this before we can merge it.

powersj · 2022-10-27T13:39:06Z

Questions before we land this:

MACHINE123,host=HOST123,id=ns=4;s=TimeEnded="2022-10-18 02:13:29.222 +0000 UTC",Quality="OK (0x0)" 1665769610000000000

Won't this increase the cardinality of a measurement greatly by having a possibly unique timestamp on every recording? If so, shouldn't this be a flag to enable this if you really want it? Or am I over estimating how often this tag value would change?

LarsStegman · 2022-10-27T13:51:00Z

Is that valid line protocol? No additional tags should be created by this PR. DateTime values for fields should become formatted as strings instead of being dropped.

powersj · 2022-10-27T14:23:08Z

TIL: I can edit your comments... that is scary

Is that valid line protocol?

I didn't think it was. I was going off of what is in the initial PR comment and made me stop. Is this just a case of a missing space?

LarsStegman · 2022-10-27T14:25:59Z

I think the field name is missing in the example. @vsinha can you confirm?

TIL: I can edit your comments... that is scary

😅

vsinha · 2022-10-27T19:28:53Z

@powersj updated the comment, agreed that as it was there was a a=b=c situation going on. Must've been a mistype from redacting the details specific to our opcua host

vsinha · 2022-10-31T16:36:34Z

@powersj @MyaLongmire anything else i should do to shepherd this PR along?

srebhan

@vsinha looks good overall, I just added one suggestion... Can you please take a look!?

plugins/common/opcua/input/input_client.go

add type assertion so we don't panic if we fail to convert to a time string Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

vsinha · 2022-11-01T17:19:11Z

updated, thanks @srebhan

vsinha · 2022-11-01T21:15:01Z

@srebhan @MyaLongmire any more pieces of feedback? or are we ready to merge

reimda

Thanks for adding this feature!

plugins/common/opcua/input/input_client.go

… into fix-opcua-datetime

LarsStegman · 2022-11-03T06:24:56Z

Nice addition for the timestamp format. A suggestion, I would put the timestamp format in the InputClientConfig, as the value is used in the InputClient, not in the Client.

telegraf-tiger · 2022-11-03T16:47:04Z

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 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_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

vsinha · 2022-11-03T17:05:46Z

done!

vsinha · 2022-11-07T19:33:15Z

Hey @srebhan @MyaLongmire @reimda -- do you think this is something we could get merged this week? Our plan is to fork telegraf to get this bug fix deployed if it's not something that can be upstreamed (via this PR) relatively quickly. I'm happy to make any more changes but I'm getting antsy.

vsinha · 2022-11-07T23:39:38Z

🙌

vsinha · 2022-12-20T22:10:21Z

@powersj @MyaLongmire could I get a copy of my countersigned CCLA for this PR? We'd like it for our own records. Thanks!

powersj · 2023-01-05T17:20:08Z

@vsinha - the form is a google docs form, so all I have is an entry in a spreadsheet.

vsinha · 2023-01-18T19:10:09Z

@powersj I think the individual CLA was a google form but the CCLA was a pdf we had our legal team sign and upload, am I remembering that correctly?

powersj · 2023-01-18T20:52:18Z

@vsinha ah yes the CCLA is different. Let me ask if I copy can get sent to you.

powersj · 2023-01-19T14:27:30Z

@vsinha what email did you send the signed CCLA? Can you send them a message to get a copy please?

fix: Add support for opcua datetime values

041f1bf

telegraf-tiger bot added the fix pr to fix corresponding bug label Oct 25, 2022

vsinha mentioned this pull request Oct 25, 2022

fix(inputs.opcua): add support for opcua datetime values #12020

Closed

fix: fixed broken test

3c5d22a

MyaLongmire mentioned this pull request Oct 26, 2022

test: multiple opcua_listener integration tests failures #12105

Closed

fix: opcua listener client integration test

15ec6ac

MyaLongmire approved these changes Oct 27, 2022

View reviewed changes

MyaLongmire added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Oct 27, 2022

vsinha mentioned this pull request Oct 31, 2022

OPCUA should support DateTime values #12006

Closed

srebhan reviewed Nov 1, 2022

View reviewed changes

plugins/common/opcua/input/input_client.go Outdated Show resolved Hide resolved

Update plugins/common/opcua/input/input_client.go

a0433f2

add type assertion so we don't panic if we fail to convert to a time string Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>

reimda suggested changes Nov 2, 2022

View reviewed changes

plugins/common/opcua/input/input_client.go Outdated Show resolved Hide resolved

Viraj Sinha added 2 commits November 2, 2022 16:38

feat(inputs.opcua): added time format option

804957a

Merge branch 'fix-opcua-datetime' of https://github.com/vsinha/telegraf…

b284fa0

… into fix-opcua-datetime

Viraj Sinha added 2 commits November 3, 2022 09:18

chore: move timestamp format to inputclient config

66e8c40

fix: remove timestamp format from ClientConfig

336664b

vsinha requested a review from reimda November 3, 2022 19:26

sspaink approved these changes Nov 7, 2022

View reviewed changes

MyaLongmire merged commit e42b083 into influxdata:master Nov 7, 2022

vsinha mentioned this pull request Dec 20, 2022

chore: update Telegraf versions for v1.25.0 influxdata/helm-charts#533

Merged

Digitomation mentioned this pull request Nov 16, 2024

fix(common.opcua): Support LocalizedText Datatype #16195

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add support for opcua datetime values #12101

fix: Add support for opcua datetime values #12101

vsinha commented Oct 25, 2022 •

edited

Loading

vsinha commented Oct 25, 2022

MyaLongmire commented Oct 25, 2022

vsinha commented Oct 25, 2022

LarsStegman commented Oct 25, 2022

LarsStegman commented Oct 26, 2022

MyaLongmire commented Oct 26, 2022

vsinha commented Oct 27, 2022

MyaLongmire commented Oct 27, 2022

MyaLongmire left a comment

powersj commented Oct 27, 2022

LarsStegman commented Oct 27, 2022 •

edited by powersj

Loading

powersj commented Oct 27, 2022

LarsStegman commented Oct 27, 2022

vsinha commented Oct 27, 2022

vsinha commented Oct 31, 2022

srebhan left a comment

vsinha commented Nov 1, 2022

vsinha commented Nov 1, 2022

reimda left a comment

LarsStegman commented Nov 3, 2022

telegraf-tiger bot commented Nov 3, 2022

Artifact URLs

vsinha commented Nov 3, 2022

vsinha commented Nov 7, 2022

vsinha commented Nov 7, 2022

vsinha commented Dec 20, 2022

powersj commented Jan 5, 2023

vsinha commented Jan 18, 2023

powersj commented Jan 18, 2023

powersj commented Jan 19, 2023

fix: Add support for opcua datetime values #12101

fix: Add support for opcua datetime values #12101

Conversation

vsinha commented Oct 25, 2022 • edited Loading

Details

Example

Before:

After:

vsinha commented Oct 25, 2022

MyaLongmire commented Oct 25, 2022

vsinha commented Oct 25, 2022

LarsStegman commented Oct 25, 2022

LarsStegman commented Oct 26, 2022

MyaLongmire commented Oct 26, 2022

vsinha commented Oct 27, 2022

MyaLongmire commented Oct 27, 2022

MyaLongmire left a comment

Choose a reason for hiding this comment

powersj commented Oct 27, 2022

LarsStegman commented Oct 27, 2022 • edited by powersj Loading

powersj commented Oct 27, 2022

LarsStegman commented Oct 27, 2022

vsinha commented Oct 27, 2022

vsinha commented Oct 31, 2022

srebhan left a comment

Choose a reason for hiding this comment

vsinha commented Nov 1, 2022

vsinha commented Nov 1, 2022

reimda left a comment

Choose a reason for hiding this comment

LarsStegman commented Nov 3, 2022

telegraf-tiger bot commented Nov 3, 2022

Artifact URLs

vsinha commented Nov 3, 2022

vsinha commented Nov 7, 2022

vsinha commented Nov 7, 2022

vsinha commented Dec 20, 2022

powersj commented Jan 5, 2023

vsinha commented Jan 18, 2023

powersj commented Jan 18, 2023

powersj commented Jan 19, 2023

vsinha commented Oct 25, 2022 •

edited

Loading

LarsStegman commented Oct 27, 2022 •

edited by powersj

Loading