telegraf icon indicating copy to clipboard operation
telegraf copied to clipboard

feat: Add input plugin for ctrlX Data Layer

Open albrecht-j opened this issue 3 years ago • 16 comments

Required for all PRs:

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

albrecht-j avatar May 20 '22 10:05 albrecht-j

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 20 '22 10:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 20 '22 14:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 20 '22 15:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 23 '22 08:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 23 '22 09:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 25 '22 15:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar May 25 '22 15:05 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar Jun 03 '22 13:06 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar Jun 07 '22 09:06 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar Jun 07 '22 10:06 telegraf-tiger[bot]

Thanks so much for the pull request! :handshake: :black_nib: 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

telegraf-tiger[bot] avatar Jun 07 '22 11:06 telegraf-tiger[bot]

Thanks @srebhan for your comprehensive review of the plugin. Indeed, this led in an enormous improvement of our implementation. Please find all comments above.

Regarding your major points:

  1. Done
  2. Done
  3. Still open, see our comment above.
  4. Done
  5. Will be implemented as a next feature after this merge with separate PR.
  6. We think the current implementation is lean and sufficient and we do not need many of the features of those 3rd party dependencies. E.g. we actually started with https://github.com/r3labs/sse but later we noticed it is far too complex and we need only a client and no server implementation.

CLA and CCLA are on its way. Will sign as soon as we are done with the code.

Thanks again, we are also looking forward to the next round ;-)

krauskopf avatar Jun 07 '22 12:06 krauskopf

Hi @srebhan, one question regarding input format parsing. We want to use a built-in parser in order to stream all data types from ctrlX DataLayer to the InfluxDB. In addition we want to use the parsers available with the property 'data_format' for advanced users. Is there already such a mechanism implemented in any input plugin? If not, how should we name the property, which switches between built-in parsing and 'data_format' parsing?

Our first approach is like this:

built-in formatting is true (default) means, data_format parser is not used:

[[inputs.ctrlx_datalayer_sse]]
  builtin_formatting =true

built-in formatting is false means, data_format parser is used:

[[inputs.ctrlx_datalayer_sse]]
  builtin_formatting  =false
  data_format         ="value"
  data_type           ="string"

DonCamillo72 avatar Jul 28 '22 13:07 DonCamillo72

Well the best way would be to write a parser and let telegraf do the magic. Otherwise, you might run into issues where use_builtin_parser is set to true, but the user also specified a data_format... If this is not an option, I would add a use_builtin_parser option that takes a boolean value...

srebhan avatar Aug 01 '22 17:08 srebhan

Hi @srebhan , here is a new commit with the suggest improvements. Please give it a go. Thanks for your patience and helpful comments. :-)

krauskopf avatar Aug 10 '22 08:08 krauskopf

Pushed the first commit with the obvious fixes. More to come in the next days. Have a nice weekend.

krauskopf avatar Aug 19 '22 16:08 krauskopf

Just committed fixes and comments for the remaining review findings. Please note, that we also have a little functional enhancement in the pipeline to make the field-names customizable. Pretty similar to how it is realized in the OPC UA plugin. Will push the feature as soon as you are fine with the current state of findings.

Thanks again for your help.

krauskopf avatar Aug 25 '22 17:08 krauskopf

@srebhan @powersj Short status info: We are currently in work to refactor the sse implementation into an external repository as you proposed. This protocol repository will be maintained by us. You may expect a new commit soon.

krauskopf avatar Oct 21 '22 08:10 krauskopf

@krauskopf any news on this PR?

srebhan avatar Jan 26 '23 10:01 srebhan

@srebhan Sry, took some time to align development of the refactorings. We are working on the topic and will provide a commit soon.

krauskopf avatar Feb 01 '23 14:02 krauskopf

Hi @srebhan , we updated the PR and you can have a look at it now. SSE is refactored to own repository as you suggested. Thanks!

krauskopf avatar Feb 09 '23 07:02 krauskopf

@srebhan as mentioned we have updated the PR. Do you have a chance to look at the PR? Thanks

albrecht-j avatar Feb 22 '23 09:02 albrecht-j

FYI March 13 is the scheduled date for v1.26.0, so I'd like to see this land if possible before then! :)

powersj avatar Feb 28 '23 20:02 powersj

Hi @albrecht-j,

Looks like it has been a bit. @srebhan has left some comments and was wondering if you were going to continue this pr.

Thanks!

powersj avatar Apr 04 '23 14:04 powersj

Hi @powersj, sorry for the delay. We are working on the comments of @srebhan. We have renamed the plugin. You can expect a new commit in the next days.

krauskopf avatar Apr 12 '23 08:04 krauskopf

Hi @srebhan , we updated the PR and fixed the issues you mentioned:

  • For the option 'output_json_string' we added some documentation in the README.md.
  • The plugin is renamed as you suggested.
  • The field name is now generated explicitly with name or implicitly with address of node configuration
  • The client Subscribe method returns now an error 'context.Canceled' and is evaluated with errors.Is(err, context.Canceled) Thanks!

DonCamillo72 avatar Apr 13 '23 11:04 DonCamillo72

Hi @srebhan, sorry for the broken PR - we synchronized the branch with the latest master sources. Please look again at our PR. Thanks!

DonCamillo72 avatar May 03 '23 11:05 DonCamillo72

Is this PR still ongoing? Can it be marked as draft until the requested changes are done?

Hipska avatar May 16 '23 14:05 Hipska

Hi @Hipska, we commited all change requests. Thanks for the really good review on our code!

DonCamillo72 avatar May 24 '23 10:05 DonCamillo72

@Hipska I'm a bit confused, is this now ready-for-final-review or do you want your comments addressed?

srebhan avatar May 25 '23 10:05 srebhan