Add statsdreceiver skeleton #566

sonofachamp · 2020-07-27T16:18:01Z

Description:

Adding the scaffolding and some basic starting parsing for a statsdreceiver plugin. It is functional, but incomplete. I'm not entirely sure what to do about aggregation yet and whether it should be included in the receiver plugin or deferred to a processor plugin, so I've started with raw mappings.

Link to tracking Issue:
#290

Testing:
There are some basic unit tests and I did some manual testing writing out ingested metrics to a file locally.

Documentation:
For now I added some basic details in the README that outlines the configuration options and supported message formats.

linux-foundation-easycla · 2020-07-27T16:18:07Z

The committers are authorized under a signed CLA.

✅ Nick (01393f6, 2034105, 7de4fcb, 88b71d1, 08abd1b, 80ec2de, 7d73e3c, 96da9f9)

codecov · 2020-07-27T19:51:36Z

Codecov Report

Merging #566 into master will increase coverage by 0.10%.
The diff coverage is 91.54%.

@@            Coverage Diff             @@
##           master     #566      +/-   ##
==========================================
+ Coverage   87.57%   87.68%   +0.10%     
==========================================
  Files         211      217       +6     
  Lines       11488    11693     +205     
==========================================
+ Hits        10061    10253     +192     
- Misses       1087     1098      +11     
- Partials      340      342       +2

Flag	Coverage Δ
#integration	`69.55% <ø> (ø)`
#unit	`87.49% <91.54%> (+0.11%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
receiver/statsdreceiver/transport/udp_server.go	`82.45% <82.45%> (ø)`
receiver/statsdreceiver/reporter.go	`86.84% <86.84%> (ø)`
receiver/statsdreceiver/transport/mock_reporter.go	`91.66% <91.66%> (ø)`
receiver/statsdreceiver/protocol/statsd_parser.go	`95.74% <95.74%> (ø)`
receiver/statsdreceiver/factory.go	`100.00% <100.00%> (ø)`
receiver/statsdreceiver/receiver.go	`100.00% <100.00%> (ø)`
processor/resourcedetectionprocessor/factory.go	`91.11% <0.00%> (-0.89%)`	⬇️
extension/observer/hostobserver/factory.go	`100.00% <0.00%> (ø)`
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19d53ca...0d26db0. Read the comment docs.

receiver/statsdreceiver/protocol/statsd_parser.go

receiver/statsdreceiver/transport/udp_server.go

keitwb · 2020-07-29T17:50:14Z

receiver/statsdreceiver/protocol/statsd_parser.go

+
+	switch metricType {
+	case "c":
+		i, err := strconv.ParseInt(metricValue, 10, 64)


Does statsd restrict counter values to only ints?

That's a good question, I'm not actually sure. I can't find anything that definitively states only ints, so I must have assumed this from all the examples showing ints. I'll expand this to support both.

According to influxdata/telegraf#556 at least consul emits floats for counter.

It looks like in that scenario it's still being cast to an int64, so it's not exactly supporting a float counter, but is able to gracefully handle it. Do you think we should do the same here or look to support Point_DoubleValue metrics?

Yes, this should support integers and floating points using either the Int64 or Double data point representation.

jmacd

This is really nice. Great work! 💯

I've written a few suggestions, but all of them can be treated as TODOs for a future PR.

jmacd · 2020-08-14T06:51:30Z

receiver/statsdreceiver/README.md

+General format is:
+
+`<name>:<value>|<type>|@<sample-rate>|#<tag1-key>:<tag1-value>,<tag2-k/v>`
+
+### Counter
+
+`<name>:<value>|c`
+
+<!-- ### Gauge
+
+`<name>:<value>|g`
+
+### Timer/Histogram
+
+`<name>:<value>|<ms/h>|@<sample-rate>` -->


The @sample-rate parameter won't have a place to go in the protocol yet, but we can leave a TODO about how to handle it.

These Timer/Histogram events should be translated the same way a ValueRecorder event from the OTel API would be, and we are meant to multiply the Count of this event by 1/sample-rate. In open-telemetry/opentelemetry-proto#159 we have discussed a potential future for OTLP exemplars that include this inverse-of-sample-rate value as well, which we termed sample-count. In other words, I like the idea of sample-rate, but I prefer to think in terms of its inverse; sample-count is a floating-point > 1, whereas sample-rate is a floating-point > 0 and < 1. (It's one less byte when sample-rate is the inverse of an integer, due to the ., which is a common case for simple probability sampling.)

What interested me the most here was that you omitted @sample-rate from Counter and Gauge events. I believe the @sample-rate may be used with Counter and Gauge events as well as Timer/Histogram events. In the PR linked above, any raw or exemplar value could include an optional sample-count > 1 to indicate that it was probabilistically sampled. There are not many (if any) definitive documents about the statsd format across the internet, so this README will be a great resource. (I think I'm asking that you add @sample-rate to the Counter and Gauge syntax above, otherwise to be proven wrong and that there exists a StatsD specification 😀 .)

Are you suggesting that we should expect @<sample-count> instead @<sample-rate> on incoming messages, or when processing incoming @<sample-rate> to translate it into sample-count? I expect sample-rate on incoming messages currently as that seems to be standard.

I will update the Counter and Gauge examples to include sample rates 👍

I expect @<sample-rate>, I was just explaining my thoughts for how to get this information into OTLP at a later date.

jmacd · 2020-08-14T06:59:39Z

receiver/statsdreceiver/protocol/statsd_parser.go

+
+	metricType := parts[1]
+
+	// TODO: add sample rate and tag parsing


Yes, let's focus on tag parsing first. 😁

I would add to this the ability to generate OTel labels from metric name patterns. For example, when monitoring AWS AppMesh (and the underlying Envoy instances), you will encounter metric names like

cluster.cds_egress_ecommerce-demo-mesh_gateway-vn_tcp_8080.upstream_rq_3xx:8|c

Where the metric name is of the form:

cluster.cds_{traffic}_{mesh}_{service}-vn_{port}.{action}

Ideally this would be an OTel metric with a name like cluster_cds_{action} with the labels being all the other pattern groups.

We have seen this not infrequently with other statsd services as well.

I looked at the Metric Transform Processor proposal but didn't see anything that would let you derive labels from a metric name pattern, but perhaps that should be part of that processor. Either way, I think this type of transformation is essential for avoiding metric name cardinality explosion and better usability of metrics.

I'd like to see if we can incorporate logic and configuration from https://github.com/prometheus/statsd_exporter for this task. Hopefully we can think of this as a separate processor stage to be combined with the statsdreceiver, and I would still prioritize parsing DogStatsd-style tags.

jmacd · 2020-08-14T07:00:38Z

receiver/statsdreceiver/protocol/statsd_parser.go

+
+	switch metricType {
+	case "c":
+		i, err := strconv.ParseInt(metricValue, 10, 64)


Yes, this should support integers and floating points using either the Int64 or Double data point representation.

receiver/statsdreceiver/receiver.go

receiver/statsdreceiver/transport/udp_server.go

sonofachamp · 2020-08-18T22:25:48Z

@bogdandrutu can we merge this? I've added TODOs for feedback from @jmacd and everything else has been addressed for now I believe.

receiver/statsdreceiver/factory.go

sonofachamp · 2020-08-19T17:01:40Z

It seems the Lint step failed due to promtheusexecreceiver unrelated to these changes:

../../Makefile.Common:110: recipe for target 'lint' failed
make[1]: *** [lint] Killed
make[1]: Leaving directory '/home/circleci/project/receiver/prometheusexecreceiver'
Makefile:91: recipe for target 'for-all-target-./receiver/prometheusexecreceiver' failed
make: *** [for-all-target-./receiver/prometheusexecreceiver] Error 2
make: *** Waiting for unfinished jobs....

Is there a way to kick off the CircleCI build manually, I don't guess I have permission to do so.

Follow up from feedback in #566

* Update README.md * Update extension/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * Update extension/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> * Update extension/README.md Co-Authored-By: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com> Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>

Nick Fischer added 8 commits June 15, 2020 13:03

Add statsdreceiver skeleton

01393f6

Add DogStatsD parser and UDP server transport

2034105

Add unit tests and copyright headers

7de4fcb

Remove unused timeout configuration option

88b71d1

Use componenterror package errors

08abd1b

Parse counters

80ec2de

Remove docker dependency from go.sum

7d73e3c

Remove unused defaultTimeout variable

96da9f9

sonofachamp requested a review from a team July 27, 2020 16:18

Nick Fischer added 2 commits July 27, 2020 12:03

Merge remote-tracking branch 'upstream/master' into statsd

ceec405

Adopt confignet.NetAddr configuration struct

80ecf24

Nick Fischer added 2 commits July 27, 2020 12:52

Fix import ordering

d8f9076

Add Reporter construct and expand test coverage

ba939df

anuraaga reviewed Jul 28, 2020

View reviewed changes

receiver/statsdreceiver/protocol/statsd_parser.go Show resolved Hide resolved

receiver/statsdreceiver/transport/udp_server.go Show resolved Hide resolved

receiver/statsdreceiver/transport/udp_server.go Show resolved Hide resolved

Improve metric name and value parsing

77fc6c4

keitwb reviewed Jul 29, 2020

View reviewed changes

Nick Fischer added 2 commits July 29, 2020 13:27

Handle float values gracefully in counter messages

0d2870e

Fill in receiver e2e test

99d53a7

jmacd approved these changes Aug 14, 2020

View reviewed changes

Nick Fischer added 3 commits August 14, 2020 12:56

Initialize transport based on configuration option

588cf6b

Update README and add TODOs from PR feedback

7fec737

Merge remote-tracking branch 'upstream/master' into statsd

c2d9ada

bogdandrutu requested changes Aug 19, 2020

View reviewed changes

receiver/statsdreceiver/factory.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into statsd

d1f7621

Nick Fischer added 2 commits August 19, 2020 11:23

Replace ReceiverFactoryOld usage in favor of receiverhelper package

df4bbe8

Merge remote-tracking branch 'upstream/master' into statsd

0d26db0

bogdandrutu approved these changes Aug 19, 2020

View reviewed changes

bogdandrutu merged commit 2559612 into open-telemetry:master Aug 19, 2020

sonofachamp deleted the statsd branch August 19, 2020 20:50

sonofachamp mentioned this pull request Aug 24, 2020

Remove goroutine spawning in favor of the async nature of UDP #794

Merged

bogdandrutu pushed a commit that referenced this pull request Aug 25, 2020

Remove goroutine spawning in favor of the asyn nature of UDP (#794)

339355d

Follow up from feedback in #566

codeboten pushed a commit that referenced this pull request Nov 23, 2022

Update AWS entry points to match spec (#566)

bddd082

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add statsdreceiver skeleton #566

Add statsdreceiver skeleton #566

sonofachamp commented Jul 27, 2020

linux-foundation-easycla bot commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading

keitwb Jul 29, 2020

sonofachamp Jul 29, 2020

keitwb Jul 29, 2020

sonofachamp Jul 29, 2020 •

edited

Loading

jmacd Aug 14, 2020

jmacd left a comment

jmacd Aug 14, 2020

sonofachamp Aug 14, 2020

jmacd Aug 14, 2020

jmacd Aug 14, 2020

keitwb Aug 14, 2020 •

edited

Loading

jmacd Aug 14, 2020

jmacd Aug 14, 2020

sonofachamp commented Aug 18, 2020

sonofachamp commented Aug 19, 2020


		metricType := parts[1]

		// TODO: add sample rate and tag parsing

Add statsdreceiver skeleton #566

Add statsdreceiver skeleton #566

Conversation

sonofachamp commented Jul 27, 2020

linux-foundation-easycla bot commented Jul 27, 2020 • edited Loading

codecov bot commented Jul 27, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonofachamp Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmacd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keitwb Aug 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonofachamp commented Aug 18, 2020

sonofachamp commented Aug 19, 2020

linux-foundation-easycla bot commented Jul 27, 2020 •

edited

Loading

codecov bot commented Jul 27, 2020 •

edited

Loading

sonofachamp Jul 29, 2020 •

edited

Loading

keitwb Aug 14, 2020 •

edited

Loading