-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(http): add functionality to retrieve/store telegraf config as toml #16132
Conversation
Also lays a foundation for future API work where the plugin list can be updated when a new telegraf version is released.
Remove notion of plugins per telegraf config and just store and serve the entire toml config file. There is backwards compatibility built in to serving the config, but saves will be performed with the new structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a massive step in the right direction, few thoughts/questions for this. Also, this may require changes to some work I'm doing in pkger
package. When this rebases off of master after todays work, it'll be pretty obvious. Give me a holler if you see breaking tests, pretty sure there will be. Happy to touch that up if needed.
http/api_handler.go
Outdated
@@ -259,6 +259,7 @@ var apiLinks = map[string]interface{}{ | |||
"tasks": "/api/v2/tasks", | |||
"checks": "/api/v2/checks", | |||
"telegrafs": "/api/v2/telegrafs", | |||
"plugins": "/api/v2/telegraf/plugins", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of me thinks this is its own resource /telegraf-plugins
, its a mouth full, but the nested /telegraf/plugins
is odd in REST, especially when we have the telegrafs
as the resource. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it that way to keep from needing an /api/v2/
route for everything telegraf related; plugins for now, possibly bundles in the future, etc... By keeping telegraf related things under the /telegraf
resource, we avoid potential future conflicts with features influxdb might add. the current /telegrafs
can't serve anything generic yet related to telegraf because the /telegrafs/:id
bit allows anything.
I'm not sure if it's "proper" as it's been a minute since i've designed api's, but i would have done something like query
for telegraf's bit:
"telegraf": map[string]string{
"plugins": "/api/v2/telegraf/plugins",
"configs": "/api/v2/telegraf/configs", # replace current `/api/v2/telegraf`
}
But, if it's "the restful way", i definitely agree this should be it's own resource, whether plugins
or telegraf-plugins
http/api_handler.go
Outdated
@@ -364,7 +365,7 @@ func (h *APIHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
if strings.HasPrefix(r.URL.Path, "/api/v2/telegrafs") { | |||
if strings.HasPrefix(r.URL.Path, "/api/v2/telegraf") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking this needs to have an addiitinal check here since we'll have both routes available now and the order matters
if strings.HasPrefix(r.URL.Path, "/api/v2/telegrafs") {
... stuffs
}
if strings.HasPrefix(r.URL.Path, "/api/v2/telegraf") {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, i think the plugins logic should use it's own handler too.
telegraf/plugins/plugin.go
Outdated
} | ||
|
||
// AvailableInputs returns the base list of available input plugins. | ||
func AvailableInputs() (*TelegrafPlugins, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the motivation for using pointer semantics as the return type here? Feels like you're better off using value semantics for the TelegrafPlugins type 🤷♂, just curious
It looks like the cypress test is failing because the bucket name isn't showing up any more on a telegraf config. where is that set and, moving forward, should it be set there/associated even? It seems like a user can manually edit their config file and add an |
Remove excessive debug log Improve unmarshalling assignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small things, but overall looks good. Might be good to get someone with more context on the frontend parts to review as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
A couple of comments - they're kind of nitpicky and assume you might not know all our TS / JS conventions. feel free to push back on them if that is wrong :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ship it!
Centralize the available telegraf plugins and expose via an api endpoint -
/api/v2/telegraf/plugins?type=[input|ouptut|processor|aggregator]
Update the telegraf config api to be plugin-unaware while still maintaining backwards compatibility with stored plugin-aware data. This is for the upcoming telegraf config editor.