-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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(outputs.datadog): Add support for submitting alongside dd-agent #15702
Changes from 1 commit
8d0f0d9
00aaf14
57ab41e
ab1fa87
c808463
d4e1381
abe5452
9954a5f
4e217d1
9bda584
f569e8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,11 +25,13 @@ import ( | |
var sampleConfig string | ||
|
||
type Datadog struct { | ||
Apikey string `toml:"apikey"` | ||
Timeout config.Duration `toml:"timeout"` | ||
URL string `toml:"url"` | ||
Compression string `toml:"compression"` | ||
Log telegraf.Logger `toml:"-"` | ||
Apikey string `toml:"apikey"` | ||
Timeout config.Duration `toml:"timeout"` | ||
URL string `toml:"url"` | ||
Compression string `toml:"compression"` | ||
ShouldRateCounts bool `toml:"should_rate_counts"` | ||
RateInterval int64 `toml:"rate_interval"` | ||
jdheyburn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Log telegraf.Logger `toml:"-"` | ||
|
||
client *http.Client | ||
proxy.HTTPProxy | ||
|
@@ -75,15 +77,15 @@ func (d *Datadog) Connect() error { | |
return nil | ||
} | ||
|
||
func (d *Datadog) Write(metrics []telegraf.Metric) error { | ||
ts := TimeSeries{} | ||
func (d *Datadog) convertToDatadogMetric(metrics []telegraf.Metric) ([]*Metric, int) { | ||
tempSeries := []*Metric{} | ||
metricCounter := 0 | ||
|
||
for _, m := range metrics { | ||
if dogMs, err := buildMetrics(m); err == nil { | ||
metricTags := buildTags(m.TagList()) | ||
host, _ := m.GetTag("host") | ||
metricType, _ := m.GetTag("metric_type") | ||
|
||
if len(dogMs) == 0 { | ||
continue | ||
|
@@ -99,9 +101,19 @@ func (d *Datadog) Write(metrics []telegraf.Metric) error { | |
dname = m.Name() + "." + fieldName | ||
} | ||
var tname string | ||
var interval int64 | ||
interval = 1 | ||
switch m.Type() { | ||
case telegraf.Counter: | ||
tname = "count" | ||
case telegraf.Counter, telegraf.Untyped: | ||
if d.ShouldRateCounts && isRateable(metricType, fieldName) { | ||
interval = d.RateInterval | ||
dogM[1] = dogM[1] / float64(interval) | ||
tname = "rate" | ||
} else if m.Type() == telegraf.Counter { | ||
tname = "count" | ||
} else { | ||
tname = "" | ||
} | ||
case telegraf.Gauge: | ||
tname = "gauge" | ||
default: | ||
|
@@ -112,7 +124,7 @@ func (d *Datadog) Write(metrics []telegraf.Metric) error { | |
Tags: metricTags, | ||
Host: host, | ||
Type: tname, | ||
Interval: 1, | ||
Interval: interval, | ||
} | ||
metric.Points[0] = dogM | ||
tempSeries = append(tempSeries, metric) | ||
|
@@ -122,6 +134,12 @@ func (d *Datadog) Write(metrics []telegraf.Metric) error { | |
d.Log.Infof("Unable to build Metric for %s due to error '%v', skipping", m.Name(), err) | ||
} | ||
} | ||
return tempSeries, metricCounter | ||
} | ||
|
||
func (d *Datadog) Write(metrics []telegraf.Metric) error { | ||
ts := TimeSeries{} | ||
tempSeries, metricCounter := d.convertToDatadogMetric(metrics) | ||
jdheyburn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if len(tempSeries) == 0 { | ||
return nil | ||
|
@@ -220,6 +238,20 @@ func verifyValue(v interface{}) bool { | |
return true | ||
} | ||
|
||
func isRateable(metricType string, fieldName string) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have the use of "metricType" in a couple places, can you add a comment to this function that explains or specifies that we are looking for metric type from statsd tag for specific cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think explicitly naming the variable // Retrieve the metric_type tag created by inputs.statsd
statsDMetricType, _ := m.GetTag("metric_type")
// ...
if d.ShouldRateCounts && isRateable(statsDMetricType, fieldName) {
// ...
func isRateable(statsDMetricType string, fieldName string) bool {
switch statsDMetricType {
case
"counter":
return true
case
"timing",
"histogram":
return fieldName == "count"
default:
return false
}
} I can then maybe put some better documentation in the README to explicitly say only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks great and makes it much clearer, thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in c808463 |
||
switch metricType { | ||
case | ||
"counter": | ||
return true | ||
case | ||
"timing", | ||
"histogram": | ||
return fieldName == "count" | ||
default: | ||
return false | ||
} | ||
} | ||
|
||
func (p *Point) setValue(v interface{}) error { | ||
switch d := v.(type) { | ||
case int64: | ||
|
@@ -246,8 +278,9 @@ func (d *Datadog) Close() error { | |
func init() { | ||
outputs.Add("datadog", func() telegraf.Output { | ||
return &Datadog{ | ||
URL: datadogAPI, | ||
Compression: "none", | ||
URL: datadogAPI, | ||
Compression: "none", | ||
RateInterval: 10, | ||
} | ||
}) | ||
} |
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.
The example config and readme should show the default values. In this case
false
and10s
. That way you don't have to document the default in the help text.I do think we should get rid of
should_rate_counts
, and use a non-zero rate_interval, or like we do in stackdriver output, we have ametric_counter = []
which means any metric matching that name will be marked as a counter. Should we do the same here and mark any metric matching that name as a rate?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.
If not getting rid of it, I would rename it to
convert_count_to_rate
or something. Using "should" in a config setting looks weird to me.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.
Appreciate the feedback.
It's a good call out, but that would not scale in larger deployments such as the one I am working with that has 1000s of metrics.
I am happy with exploring having
rate_interval
default to 0, and if it is > 0 then enable conversion of counts to rates. My concern would be if users would know to set it to 10 - which is the default rate interval used by the Datadog agent - I can explicitly state this in the README though.If it were to stay then I have no objections to changing
should_rate_counts
toconvert_count_to_rate
.Let me know your thoughts 👍🏻
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'd still like to see the single option and you can document the suggested interval would be 10 seconds as it is default used by Datadog. Sound good?
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.
SGTM - I've made the change to use single option in c808463. I also added a test in there for the status quo when
rate_interval=0
👍🏻