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 support for configurable OTLP export strategy with cumulative default strategy #1

Merged
merged 8 commits into from
Aug 18, 2020

Conversation

huyan0
Copy link

@huyan0 huyan0 commented Aug 5, 2020

This PR describes and records discussions around spec #731, #725 and the overall conclusion from Metrics SIG meetings.

cc: @alolita @huyan0

@huyan0 huyan0 changed the title Add support for configurable OTLP export strategy in the SDK Add support for configurable OTLP export strategy with cumulative default strategy Aug 6, 2020
Copy link

@CunjunWang CunjunWang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

This is 💯 great!


To support Prometheus as well as delta backends, the SDK should be configurable and supports OTLP exporter to have both cumulative default or delta export behavior. The implication is that the OTLP metric protocol should support both cumulative and delta report strategy.

Users should be allowed to declare an environment variable or configuration field that determines this setting for OTLP exporters.
Copy link

Choose a reason for hiding this comment

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

Maybe suggest a value?


To support cumulative export strategy, the SDK needs to maintain state of each cumulative metrics. This means users with high-cardinality metrics can experience high memory usage.

This problem could be addressed by adding support in the Collector, when configured as an Agent, to support converting delat OTLP to Cumulative OTLP, but this requires a single agent for each metric generating client so that all delta values of a metric are converted at the same location.
Copy link
Author

Choose a reason for hiding this comment

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

typo: delta


## Motivation

1. Metric backends such as Prometheus and Prometheus Remote Write integrated storages requires cumulative values for cumulative/adding metrics rather than delta value per collection interval. Thus, to export metrics generated by the SDK using the collector, incoming values from the SDK should be in cumulative form. However, backends like Statsd expects delta value of each collection interval. To support different backends, OTLP metric export behavior should be configurable, with cumulative export as default. The metrics SIG meeting concluded that cumulative export behavior is more reliable, as explained in the following section.

Choose a reason for hiding this comment

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

Maybe say "backends that make use of the Prometheus Remote Write API"

Copy link

Choose a reason for hiding this comment

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

OTLP Configurable Export Behavior

Add support for configurable behavior in OTLP exporters

Based on discussions from the Metrics SIG meeting, configurable export behavior from the OTLP exporter is desired to support different mertic backends
huyan0 and others added 5 commits August 10, 2020 20:58
Based on discussions from the Metrics SIG meeting, configurable export behavior from the OTLP exporter is desired to support different mertic backends
open-telemetry#126)

* Add a proposal for SDK configurations for metric aggregation.

* rename file to match the PR #

* fix markdown lint issues

* clarify that this applies to the default SDK, and fill out the open questions

* update the java example to fix the naming changes

* Add another open question to the list.

* Update text/0126-Configurable-Metric-Aggregations.md

Co-authored-by: Chris Kleinknecht <libc@google.com>

* Update to use the 'view' terminology

* another configuration/view replacement

* Add a few more open questions, and a note that they will be resolved in the spec.

* Update text/0126-Configurable-Metric-Aggregations.md

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Co-authored-by: Chris Kleinknecht <libc@google.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com>
@huyan0 huyan0 merged commit f4555e8 into open-o11y:master Aug 18, 2020
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.

8 participants