telegraf
telegraf copied to clipboard
influxdata
feat: Add Azure Monitor input plugin
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
resolves #7930
Hi @srebhan,
I changed the code according your comments.
Happy holidays :)
@ShiranAvidov if you could please rebase on master and resolve the conflicts with go.mod that would help keep this PR moving.
Thanks!
:relaxed: This pull request doesn't significantly change the Telegraf binary size (less than 1%)
:package: Looks like new artifacts were built from this PR.
Expand this list to get them here ! :tiger:
Artifact URLs
![telegraf-tiger[bot] avatar](https://avatars.githubusercontent.com/in/94907?v=4 "Published by a pub.dev verified publisher"){kind=small}
I am working on using the azure sdk for resources and collecting metrics. I will update soon.
@ShiranAvidov thanks for looking into the SDK. Please also note that they have zillions of packages, so if you miss some functionality maybe also look at the neighboring packages. :-) Thanks again for your effort!
Hey @srebhan, hope you are doing well. I finished updating the code + tests.
Hi @ShiranAvidov. I'm glad to see a PR open for a plugin I was looking for. I played around with this a bit and have a few ideas.
I'm concerned over the timeStamp label causing high cardinality. I'd be curious to know @srebhan opinion on this. For now I'm using fielddrop in my config to get rid of it.
And then a feature idea here (not something that needs to be in this PR, it will probably require a bit of work) could be the ability to backfill data. Right now it looks like every interval it grabs the latest data point. A single API call is capable of pulling a days worth of datapoints. Instead of making an API call every minute to get 1 minute metric resolution, it'd be cool if I were able to set the interval to a greater time and it backfill data for that entire interval. This would save a lot of cost on API calls for when I don't necessarily need the data to be as live as possible. I will admit there's some nuances to a feature like this that probably make it easier said than done. Off the top of my head I can think of interval jitter causing there to be overlapping datapoints.
Thanks for making this plugin. It's simple to use, works well, and I have a use case for something just like this.
@ShiranAvidov Cool helpful plugin. I'm looking forward to see the new functionalty in the near future inside the Telegraf.
Honestly speaking an integration would require to dramatically reduce the code size by either using existing libraries or move the inner mechanisms to a "high-level" library hiding the complexity. The present PR has 3.8k of code where around 1.2k is actual functionality and the remaining 2.6k is testing of internal functionality. That's about three times the size of most plugins. I really think this should be a library on it's own (or using an existing library if there is any) and a lean telegraf plugin only calls into this externally maintained lib...
Hi @srebhan, I've checked the code a bit deeper and I fully agree that is necessary to clean up/ refactor the code. I've quickly check it and I think it's possible to reduce the real code size to less than 1000 lines. Hi @ShiranAvidov, feel free to ping me if you need support to refactor/ restructure the code.
@infa-pavakuma as stated earlier, the PR needs a revamp to reduce the amount of code by either using existing libraries or by moving most of the inner workings to a library. When this happened and the PR is updated I will give it another review. Asking when this will be merged won't speed-up things, but supporting @ShiranAvidov and @ZPascal might... ;-)
@srebhan @ZPascal thanks for the feedback. @ZPascal can you please elaborate more about your suggestions to reduce the size of the code? Currently I am using Azure library that lets me send requests to the Azure monitor API.
@ShiranAvidov and @ZPascal please keep this going! Just ping me if you need my input...
@ShiranAvidov I checked your code a bit, and it seems that you have some pre-existing (SDK) methods wrapped in your codebase. I recommend reducing the code size also by separating basic lines of code in a separate/unified library. In the end, it is possible to use this common library within the Telegraf codebase. My offer still stands, if you need assistance with this, we can implement it together.
Hey @srebhan, I reduced the size of the code. The code is in one file - 235 lines!!!
Hey @srebhan, sorry had a lot of other work to do. I think I can move the data into json files, but I will still have to use the List methods (the load/unmarshal will be in the List methods).
Hey @srebhan, finished fixing the code. I moved all test data to json/toml files as you requested :)
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. Downloads for additional architectures and packages are available below.
:relaxed: This pull request doesn't significantly change the Telegraf binary size (less than 1%)
:package: Click here to get additional PR build artifacts
Artifact URLs
@srebhan I think this one looks good and is ready. It does not have the sample config structure we use now, were you planning to do that in a follow up?
@powersj Is there the possibility to rebase and merge it? We want to consume the new functionality.
please run go mod tidy and check in changes, you might have to use the same version of Go as the CI
Doesn't look like it, so still need you to push an update after running go mod tidy
@srebhan can you re-reivew, I've cleaned up linter issues, ran make tidy, and moved the sample config out