feat(inputs.mikrotik): Add plugin #16081
Conversation
|
Thanks so much for the pull request! |
s-r-engineer
force-pushed
the
master
branch
3 times, most recently
from
e557986 to
b9c5115
Compare
telegraf-tiger
bot
added
the
feat
|
This three issues I am having:
|
#16087 I believe should fix the first two, and the 3rd is an unrelated flaky test, so nothing needed here (other than a rebase/merge after the linked PR merges) |
telegraf-tiger
bot
added
the
plugin/input
There was a problem hiding this comment.
Thanks @s-r-engineer for your contribution! I want to encourage you to simplify your code! Why not defining known tag and field names of responses and decode the JSON responses to map[string]string? Then you can iterate over the map and decode the value accordingly? This would get rid of the reflection code and in general would simplify the code...
|
@s-r-engineer my review comments are that, comments about the code. They are not meant to criticize or insult you as a programmer or anyone else. If you understood it this way, please accept my apologies. |
@srebhan I never was insulted whatsoever. I am sorry if I made you feel like that. |
s-r-engineer
force-pushed
the
master
branch
2 times, most recently
from
0edceab to
d1f2e40
Compare
|
@srebhan what to do with this? I am not sure why this is an issue. https://app.circleci.com/jobs/github/influxdata/telegraf/363717 |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
@srebhan shall I fix something else? |
There was a problem hiding this comment.
@s-r-engineer thanks for the update! We are coming closer... I still do have some comments especially around redirections in your code. Please do understand that we need to maintain this potentially "forever" and therefore need to enforce a certain style in the code otherwise debugging or extending this later will be much harder for us. We do have these examples in our codebase already and cannot extend that zoo infinitively.
| type common []commonData | ||
|
|
||
| type commonData map[string]string | ||
|
|
There was a problem hiding this comment.
Please get rid of this indirection!
There was a problem hiding this comment.
- Why?
- I need both because some endpoints are returning not arrays. Or are you suggesting to not to have commonData named at all?
| } | ||
| } | ||
|
|
||
| points = append(points, parsedPoint{Tags: tags, Fields: fields}) |
There was a problem hiding this comment.
You could create the metric here, can't you? This way you can get rid of the extra struct and/or directly add them to the accumulator.
There was a problem hiding this comment.
I need then pass system tags here or redo parset to be a method of the type. I can redo that into yield if that is ok
plugins/inputs/mikrotik/parsers.go
Outdated
| ) | ||
|
|
||
| func parse(data common) (points []parsedPoint, err error) { | ||
| var errorList []string |
There was a problem hiding this comment.
Why not keep the errors as errors and maybe directly log them or if you need to join them in the end using errors.Join?
There was a problem hiding this comment.
Ok. What structure do you prefer? To have a struct with everything being a part of it? Or to have whatever possible not a part of it?
|
#16081 (comment) |
s-r-engineer
force-pushed
the
master
branch
2 times, most recently
from
9118e53 to
863a3ac
Compare
|
@s-r-engineer even though I do believe you that this is your intention, I've seen the opposite in the past, e.g. because people moved away from the hardware or due to other personal reasons.... Any way, I'm very happy with you maintaining that plugin! Let me know as soon as you are ready for the next review round... |
Hi, @srebhan |
| parsedData, err := parse(result) | ||
| if err != nil { | ||
| return fmt.Errorf("gatherURL -> %w", err) | ||
| } | ||
|
|
||
| for _, point := range parsedData { | ||
| point.Tags["source-module"] = endpoint.name | ||
| for n, v := range h.tags { | ||
| point.Tags[n] = v | ||
| } | ||
| acc.AddFields("mikrotik", point.Fields, point.Tags, timestamp) | ||
| } |
There was a problem hiding this comment.
Looks like this parse method could already return a telegraf metric instead of something that almost looks like a metric (only Tags and Fields).
| parsedData, err := parse(result) | |
| if err != nil { | |
| return fmt.Errorf("gatherURL -> %w", err) | |
| } | |
| for _, point := range parsedData { | |
| point.Tags["source-module"] = endpoint.name | |
| for n, v := range h.tags { | |
| point.Tags[n] = v | |
| } | |
| acc.AddFields("mikrotik", point.Fields, point.Tags, timestamp) | |
| } | |
| metrics, err := parse(result, endpoint.name) | |
| if err != nil { | |
| return fmt.Errorf("could not parse result: %w", err) | |
| } | |
| for _, metric := range metrics { | |
| acc.AddMetric(metric) | |
| } |
It also looks like the name for the metric should be endpoint.name. Currently, You are only putting that into a tag, meaning there will be a lot of metrics with the same name but with totally different tags- and fields-set.
| ## Optional TLS Config | ||
| # tls_ca = "/etc/telegraf/ca.pem" | ||
| # tls_cert = "/etc/telegraf/cert.pem" | ||
| # tls_key = "/etc/telegraf/key.pem" | ||
| ## Use TLS but skip chain & host verification | ||
| # insecure_skip_verify = false |
There was a problem hiding this comment.
Use config_includer/generator, see http input as example.
| Username config.Secret `toml:"username"` | ||
| Password config.Secret `toml:"password"` | ||
| Log telegraf.Logger `toml:"-"` | ||
| common_tls.ClientConfig |
There was a problem hiding this comment.
Probably want to use client config from plugins/common/http?
| common_tls.ClientConfig | |
| http.HTTPClientConfig |
| var sampleConfig string | ||
|
|
||
| type Mikrotik struct { | ||
| Address string `toml:"address"` |
There was a problem hiding this comment.
Is it an idea to support an array of URLs?
|
|
||
| func (h *Mikrotik) Init() error { | ||
| if h.Username.Empty() { | ||
| return errors.New("mikrotik init -> username must be present") |
There was a problem hiding this comment.
| return errors.New("mikrotik init -> username must be present") | |
| return errors.New("username is required") |
|
|
||
| for _, selectedModule := range h.IncludeModules { | ||
| if _, ok := modules[selectedModule]; !ok { | ||
| return fmt.Errorf("mikrotik init -> module %s does not exist or has a typo. Correct modules are: %s", selectedModule, getModuleNames()) |
There was a problem hiding this comment.
| return fmt.Errorf("mikrotik init -> module %s does not exist or has a typo. Correct modules are: %s", selectedModule, getModuleNames()) | |
| return fmt.Errorf("module %s does not exist or has a typo. Correct modules are: %s", selectedModule, getModuleNames()) |
These error prefixes are not needed, just make sure the rest of the message is specific enough.
Do you maybe not destroy the secret-buffer? |
You mean I shall destroy that or shall not but desroying anyway? |
|
@s-r-engineer did you mean to close this PR? I will need to take a look into the "secret" issue. |
|
Yes I meant to close this. I am tired of this and kinda feel that until all telegraf maintainers would not review my code it will not be published. Since I am willing to suport my code in the future I do not see any actual reason to merge it to telegraf in this circumstances. I will publish my module as a standalone daemon that could be running with execd. |
|
@s-r-engineer would you be OK if I work on the code to get it to a mergable state? You are so close and there are only small things left to do... I would do it on your branch if you provide write access or in a separate branch if you are ok with it... |
|
@srebhan no I'm not ok with it. I deleted my clone of the repo already and moved the code to standalone binary that will not be covered with MIT from now on. But your repo is covered by MIT license and I signed CLA. Therefore ultimately you can do whatever you want with the blocks I already submitted. |
|
We will respect your choice of course and not work on the plugin. If you decide you want to add the plugin upstream one day, please do not hesitate to reopen the PR or add a new one. I'm sad we seem to have scared you away from adding this plugin and I would be interested in learning what we did wrong... |
Summary
The idea is to be able to connect and gather metrics from the Mikrotik's RouterOS
Checklist
Related issues
resolves #16080