Dynatrace Plugin: Make conversion to counters possible / Changed large bulk handling #8397
Conversation
Add Dynatrace Plugin
Add oneagent handling
Co-authored-by: Steven Soroka <ssoroka78@gmail.com>
Co-authored-by: Steven Soroka <ssoroka78@gmail.com>
|
@thschue Is this PR ready for review? Still see some |
The PR is ready now |
| @@ -219,6 +232,15 @@ func (d *Dynatrace) Write(metrics []telegraf.Metric) error { | |||
| default: | |||
| fmt.Fprintf(&buf, "%s%s %v\n", metricID, tagb.String(), value) | |||
| } | |||
|
|
|||
| if metricCounter%1000 == 0 { | |||
There was a problem hiding this comment.
Shouldn't 1000 be configurable or at least not used as a magic number?
There was a problem hiding this comment.
This is to ensure that the bulks sent to the api are not too large (that's also the reason, why it isn't configurable)
There was a problem hiding this comment.
keep in mind if this splits it into chunks and the last chunk fails, all chunks will be retried(re-sent) when returning an error. When returning a nil, no chunks will be retried(re-sent).
I wonder if instead we should allow plugins to request that the batch size not exceed a certain limit.
There was a problem hiding this comment.
Since that isn't possible now, I think the solution here is sufficient. 1000 is a hard limit set by the dynatrace api rate limiting so a configuration wouldn't do any good.
Retrying the whole batch if the last chunk fails is unfortunate and something i'm taking a note of that I might fix in a follow up. Thanks for the heads-up.
There was a problem hiding this comment.
Hey @zak-pawel! You still have a request changes in place for this comment. Would you be fine with merging this now? Thanks.
There was a problem hiding this comment.
@arminru I understand why it should not be configurable.
But I don't understand why this magic number cannot be converted to const. You can name it dynatraceAPIRateLimit (or similar) and everyone will be happy :)
There was a problem hiding this comment.
Sure, making it a const for better readability makes sense - will do!
There was a problem hiding this comment.
@arminru Now it's great.
There was a problem hiding this comment.
Thanks!
@ssoroka can this be merged now? 🙂
|
see last comment |
|
Hey, any chance this could be merged? Thanks! |
|
@helenosheaa @ssoroka I updated the PR to pick up the latest Revive fixes from the master branch. |
Required for all PRs:
Some input plugins do not set a ValueType for counters, which causes them to be handled as gauges in dynatrace. This PR enables the definition of such metrics to be handled as counters