Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow timestamping gnmi notifications locally #8420

Closed
wants to merge 1 commit into from

Conversation

rski
Copy link

@rski rski commented Nov 17, 2020

Sometimes it can be useful to use a local, on receive timestamp for
gnmi notifications. This is the case for sample subscriptions. The
gnmi spec requires that the update timestamps are the time when the
value from the underlying data source (e.g. counter hardware) changed.
This value might not change for a long time, resulting in a timestamp
way back in the past. For such situations, it can be useful to use the
current time on the server, turning the semantics of the timestamp
into "This was the value on the device at time T according to the
collector" from "This was when the value last changed on the device."

Fixes #8411

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@sjwang90 sjwang90 added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/gnmi labels Nov 17, 2020
@srebhan
Copy link
Contributor

srebhan commented Nov 19, 2020

@rski thanks for submitting this PR. Unfortunately it seems you have not yet signed the CLA which prevents proper review of your changes. Could you please fill the CLA!

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Please also add a description of the newly added option to the README.md file and to the sampleConfig variable.

@srebhan srebhan self-assigned this Nov 20, 2020
@rski
Copy link
Author

rski commented Nov 20, 2020

@rski thanks for submitting this PR. Unfortunately it seems you have not yet signed the CLA which prevents proper review of your changes. Could you please fill the CLA!

I will, I'm in the process of getting the CCLA signed too. I'll update the README as well, I knew I was forgetting something :)

@srebhan
Copy link
Contributor

srebhan commented Nov 21, 2020

No worries. Looks quite good already. :-)

@rski
Copy link
Author

rski commented Nov 23, 2020

ok, I updated the docs, I'll let you know when I have an update on the CCLA. That will probably be next week.

@srebhan
Copy link
Contributor

srebhan commented Nov 25, 2020

Code looks good. Waiting for the administrative requirements... :-)

Sometimes it can be useful to use a local, on receive timestamp for
gnmi notifications. This is the case for sample subscriptions. The
gnmi spec requires that the update timestamps are the time when the
value from the underlying data source (e.g. counter hardware) changed.
This value might not change for a long time, resulting in a timestamp
way back in the past. For such situations, it can be useful to use the
current time on the server, turning the semantics of the timestamp
into "This was the value on the device at time T according to the
collector" from "This was when the value last changed on the device."

Fixes influxdata#8411
@rski
Copy link
Author

rski commented Dec 7, 2020

the CCLA from Arista and my personal CCL are now all submitted, I gave the changes a rebase + conflict resolution just now to the latest telegraph master

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

LGTM

@srebhan srebhan 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 Dec 11, 2020
@rski
Copy link
Author

rski commented Dec 17, 2020

gentle bump?

@ssoroka
Copy link
Contributor

ssoroka commented Dec 18, 2020

I'm wondering if this should just be a processor plugin. This comes up many times across a lot of different plugins.

@srebhan
Copy link
Contributor

srebhan commented Dec 18, 2020

@rski you've not been forgotten. :-) The guys are about to release 1.17 and PRs merges are held back until the release is done AFAIK.

@ksator
Copy link

ksator commented Jan 6, 2021

@srebhan hello, several of us are looking forward for this merge :-)
Many thanks in advance for your help.

@srebhan
Copy link
Contributor

srebhan commented Jan 11, 2021

@ksator well I'm only the poor reviewer and that's where my power ends... ;-)
@ssoroka can we merge this for now until we solve this more generally?

@ssoroka
Copy link
Contributor

ssoroka commented Jan 13, 2021

I think we're going to go in a different direction with this, but if you want to use this branch until another solution is ready, you can grab the relevant build artifact directly and use it until the other solution is ready.

https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.aarch64.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.armel.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.armv6hl.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.i386.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.ppc64le.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.s390x.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632-0.x86_64.rpm
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_darwin_amd64.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_freebsd_amd64.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_freebsd_i386.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_amd64.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_arm64.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_armel.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_armhf.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_i386.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_mips.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_mipsel.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_ppc64le.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_linux_s390x.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_static_linux_amd64.tar.gz
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_windows_amd64.zip
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf-1.16.3%7E9ed9a632_windows_i386.zip
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_amd64.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_arm64.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_armel.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_armhf.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_i386.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_mips.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_mipsel.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_ppc64el.deb
https://74283-33258973-gh.circle-artifacts.com/0/build/dist/telegraf_1.16.3%7E9ed9a632-0_s390x.deb

@sjwang90
Copy link
Contributor

@rski Thanks so much for your PR but as the comment above mentioned we thought we should go another route that is outlined in #8689.

I'll leave this PR open until #8689 gets addressed.

@sjwang90 sjwang90 removed 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 Jan 13, 2021
@sjwang90
Copy link
Contributor

If anyone is looking for this feature on gnmi specifically, pleas use the builds from above. I would like to close this for now.

If anyone would like to work on #8689 for local timestamping to be used across plugins more generally we would gladly welcome a PR.

@sjwang90 sjwang90 closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gnmi feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

collector-local timestamps for gnmi plugin
5 participants