Skip to content

[com.influxdata.telegraf] Add a few input plugins#196

Merged
bioball merged 2 commits into
apple:mainfrom
in-the-mood-for-mov:main
Jun 8, 2026
Merged

[com.influxdata.telegraf] Add a few input plugins#196
bioball merged 2 commits into
apple:mainfrom
in-the-mood-for-mov:main

Conversation

@in-the-mood-for-mov

Copy link
Copy Markdown
Contributor

Hi!

This is pull request that adds a few of the simpler input plugins for Telegraf.

  • diskio
  • kernel
  • mem
  • processes
  • redis
  • swap
  • system

I'm on the fence about two things :

  • Should I split this commit in multiple commits and/or PRs?
  • I looked at the existing plugins, some were open modules and some were closed. I decided to open those that were non-trivial to let the user add configurations that could be out of sync. Maybe I should open all of them instead?

Thank you

@HT154 HT154 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! Just one issue in the redis input.

RE: your questions:

  • I have no objection to this being one PR; it's of a reasonable size and complexity to review in one go.
  • These should be closed modules. I'm not 100% sure why some of the existing ones are open, but it's likely historical (and would be a breaking change to fix).

Comment on lines +57 to +67
/// The command to send to the Redis server, given as the command name
/// followed by its arguments.
///
/// For example, `new { "get"; "sample-key" }`.
command: Listing<String>?

/// The field to store the result in.
field: String?

/// The type of the result.
type: "string" | "integer" | "float" | Null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reading the plugin source, it appears that all of these properties are actually required:

Suggested change
/// The command to send to the Redis server, given as the command name
/// followed by its arguments.
///
/// For example, `new { "get"; "sample-key" }`.
command: Listing<String>?
/// The field to store the result in.
field: String?
/// The type of the result.
type: "string" | "integer" | "float" | Null
/// The command to send to the Redis server, given as the command name
/// followed by its arguments.
///
/// For example, `new { "get"; "sample-key" }`.
command: Listing<String>
/// The field to store the result in.
field: String
/// The type of the result.
type: "string" | "integer" | "float"

* diskio
* kernel
* mem
* processes
* redis
* swap
* system
@in-the-mood-for-mov

Copy link
Copy Markdown
Contributor Author

Thank you for the review, I updated the PR. One question I had that I forgot to ask : for properties such as RedisInput.Servers, I considered using a Listing<Uri>? (with URI from pkl.experimental.uri, not the type alias in base), but decided not to because it would add a dependency. What do you think?

@bioball

bioball commented Jun 8, 2026

Copy link
Copy Markdown
Member

Yeah, it makes sense to use what's available in the stdlib rather than pkl.experimental.uri (that module provides a parser and a structured way to define URIs).

@bioball bioball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, this is great, thank you!

@bioball bioball merged commit c124695 into apple:main Jun 8, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants