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

Add option to set Counter to be monotonic #4154

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

locmai
Copy link
Contributor

@locmai locmai commented Jul 12, 2021

Description:
Detailed bug: #4153

This will provide the option to set counter metric to become monotonic and therefore become cumulative (AggregationTemporalityCumulative) instead of AggregationTemporalityDelta - which will be dropped by prometheusexporter.

Link to tracking Issue: #4153

Testing:

Build and run the otel-collector with the following config.yaml configuration:

receivers:
  statsd:
    endpoint: "0.0.0.0:8127"
    aggregation_interval: 5s
    enable_metric_type: true
    is_monotonic_counter: true
    timer_histogram_mapping:
      - statsd_type: "histogram"
        observer_type: "summary"
      - statsd_type: "timing"
        observer_type: "summary"
processors:
  batch:
exporters:
  prometheus:
    endpoint: "0.0.0.0:9090"
    metric_expiration: 180m
  file:
    path: ./metrics.json
service:
  pipelines:
    metrics:
      receivers: [statsd]
      processors: [batch]
      exporters: [prometheus,file]

Send the test command:

echo "locmai.test:30|c" | nc -w 1 -v -u 0.0.0.0 8127

The metric will be exported on prometheus exporter and file exporter as well:

metrics.json:

{"resourceMetrics":[{"resource":{},"instrumentationLibraryMetrics":[{"instrumentationLibrary":{},"metrics":[{"name":"locmai.test","intSum":{"dataPoints":[{"labels":[{"key":"metric_type","value":"counter"}],"timeUnixNano":"1626103559426960000","value":"30"}],"aggregationTemporality":"AGGREGATION_TEMPORALITY_CUMULATIVE","isMonotonic":true}}]}]}]}

prometheus:

# HELP locmai_test 
# TYPE locmai_test counter
locmai_test{metric_type="counter"} 30

Documentation:
I updated the option in README.md of the statsdreceiver

@locmai locmai requested review from jmacd and a team as code owners July 12, 2021 15:38
@locmai locmai requested a review from jrcamp July 12, 2021 15:38
@project-bot project-bot bot added this to In progress in Collector Jul 12, 2021
Collector automation moved this from In progress to Reviewer approved Jul 12, 2021
@@ -21,6 +21,8 @@ The Following settings are optional:

- `enable_metric_type: true`(default value is false): Enable the statsd receiver to be able to emit the metric type(gauge, counter, timer(in the future), histogram(in the future)) as a label.

- `is_monotonic_counter` (default value is false): Set all counter-type metrics the statsd receiver received as monotonic.
Copy link
Member

Choose a reason for hiding this comment

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

Is this true all the time? Do we need to have per counter config?

Copy link
Contributor Author

@locmai locmai Jul 12, 2021

Choose a reason for hiding this comment

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

Do we need to have per counter config?

With the current change, it applies to all counters.

I think we do need for per counter config, we could add include/exclude with regex? Although it feels more like a processor job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. In previous PRs on this receiver I've asked that we follow the Prometheus statsd exporter's configuration, e.g.,
https://github.com/prometheus/statsd_exporter#glob-matching

I think it is fine to add this to a TODO and continue in another PR. The current PR allows a default choice, and I would say that probably the default should be true (i.e., assume monotonic by default).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it would be great if this receiver were following the statsd exporter 👍🏼

May I have this one merged until that come? I set the default to false since seeing this #1789 so I don't want to cause any trouble for the DataDog exporter.

@bogdandrutu bogdandrutu merged commit 57676a9 into open-telemetry:main Jul 20, 2021
Collector automation moved this from Reviewer approved to Done Jul 20, 2021
mstumpfx pushed a commit to mstumpfx/opentelemetry-collector-contrib that referenced this pull request Aug 31, 2021
* Add option to set Counter to be monotonic

* update tests

* update tests 2

* update tests 3
hex1848 pushed a commit to hex1848/opentelemetry-collector-contrib that referenced this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants