Skip to content
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

[Statsd receiver]Add metric type as a label #2466

Merged

Conversation

JohnWu20
Copy link
Contributor

@JohnWu20 JohnWu20 commented Feb 25, 2021

Backgrounds:

One of our customer has a requirement to get a metric_type label. This label is same with the statsd data type.
Because this dimension is only required by one of our customer, we decide to add flag to enable this label in the config file.

Description:

1, Add a new label called metric_type for statsd receiver.

2, Add a new flag label for the config file of the statsd. enable_metric_type

3, Add unit tests for the config when enable_metric_type = true

Testing:
Unit test and local test.

@JohnWu20 JohnWu20 requested a review from jmacd as a code owner February 25, 2021 06:20
@JohnWu20 JohnWu20 requested a review from a team February 25, 2021 06:20
@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #2466 (2c0bc18) into main (70d8303) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2466      +/-   ##
==========================================
+ Coverage   90.91%   90.93%   +0.01%     
==========================================
  Files         411      411              
  Lines       20467    20481      +14     
==========================================
+ Hits        18608    18624      +16     
+ Misses       1397     1396       -1     
+ Partials      462      461       -1     
Flag Coverage Δ
integration 69.22% <ø> (ø)
unit 89.78% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
receiver/statsdreceiver/factory.go 100.00% <100.00%> (ø)
receiver/statsdreceiver/protocol/statsd_parser.go 98.02% <100.00%> (+0.18%) ⬆️
receiver/statsdreceiver/receiver.go 93.22% <100.00%> (ø)
receiver/k8sclusterreceiver/watcher.go 97.64% <0.00%> (+2.35%) ⬆️

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 70d8303...2c0bc18. Read the comment docs.

Copy link
Contributor

@gavindoudou gavindoudou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, two points:

  1. Need to revise Readme.
  2. It's better to explain a little bit more about the change in description so that Josh can have some background here.
  3. You can ask Anuraag to take a look and get some comments.

@PettitWesley
Copy link

minor thing: I think label is the correct OT terminology not dimension?

@PettitWesley
Copy link

The description says the field is metric_type_enable but the code has enable_metric_type. I think enable_metric_type is better.

@JohnWu20 JohnWu20 changed the title [Statsd receiver]Add metric type as a dimension [Statsd receiver]Add metric type as a label Feb 26, 2021
@JohnWu20
Copy link
Contributor Author

minor thing: I think label is the correct OT terminology not dimension?

Thanks Wesley, updated the description and title.

@jmacd
Copy link
Contributor

jmacd commented Feb 27, 2021

I wonder if this could be inferred in the exporter based on the type of OTLP data type that is used. Then, the statsd receiver would do the ordinary thing, and the EMF exporter can annotate the instrument type by inference. I think other metric exporters will gain enough information based on the OTLP type. The mapping is:

OTLP Sum point: Statsd type "counter"
OTLP Gauge point: Statsd type "gauge"
OTLP Histogram or Summary point, where units are "ms": Statsd type "timing"
OTLP Histogram or Summary point, where units are not "ms": Statsd type "histogram"

Would that be sufficient?

@PettitWesley
Copy link

@jmacd we'd have to build statsd specific logic into our exporter which would look at each metric and then append this label.

At least just from a software engineering standpoint, that feels wrong. The statsd specific logic of adding the right statsd metric type as a label seems best placed in the statsd receiver itself.

@gavindoudou
Copy link
Contributor

I wonder if this could be inferred in the exporter based on the type of OTLP data type that is used. Then, the statsd receiver would do the ordinary thing, and the EMF exporter can annotate the instrument type by inference. I think other metric exporters will gain enough information based on the OTLP type. The mapping is:

OTLP Sum point: Statsd type "counter"
OTLP Gauge point: Statsd type "gauge"
OTLP Histogram or Summary point, where units are "ms": Statsd type "timing"
OTLP Histogram or Summary point, where units are not "ms": Statsd type "histogram"

Would that be sufficient?

Hi Josh, we have two reasons that we need to add these tags:

  1. for ms and h, we may use the same type to send to the backend. backend cannot distinguish.
  2. Now, we are using OpenCensus metrics types, it would be better if we have that.
    Also. the default value is false, it does not change any behavior.

@JohnWu20
Copy link
Contributor Author

Confused about the CircleCI unit test, before I updated the read me, the all the checks were successful, but now the windows unit test for emf-exporter failed twice. Should I create an issue about that?

@jmacd
Copy link
Contributor

jmacd commented Feb 27, 2021

I won't object. If you want these labels eventually it has to be configured somewhere, but I had to ask.

for ms and h, we may use the same type to send to the backend. backend cannot distinguish.

With both OTLP and the OC types there is a unit field that can carry the distinction between timing and histogram.

Now, we are using OpenCensus metrics types

Great, let's fix that 😀 😀 😀

Re run unit test
Re-run integration test
This was referenced Mar 15, 2021
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
* Add the metric type as a dimension for StatsD receiver

* Update readme

* Fix a typo

* re-run test

* Re-run test

* Update statsd_parser.go

Re run unit test

* Re-run integration test

Re-run integration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants