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

Support configurable AggregationTemporality in exporters; add OTLP missing sum point temporality/monotonic fields #1296

Merged
merged 10 commits into from
Nov 10, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Oct 30, 2020

The metric export API has a type ExportKind that describes which aggregation temporality to use, Delta or Cumulative. There was a special value, PassThroughExporter, indicating the intent to use the natural aggregation temporality for sums, since that leads to a stateless configuration. The associated type ExportKindSelector is used to choose an export kind, and there was confusion over actual export kinds (Delta or Cumualtive) and selectors, which are policies that decide the export kind.

This PR removes PassThroughExporter, replacing it with StatelessExportKindSelector(). This makes it so that ExportKind does not implement ExportKindSelector, which was being used for constant ExportKindSelector policies (and made the code difficult to read). Constant policies are now available as DeltaExportKindSelector() and CumulativeExportKindSelector().

Updates the OTLP exporter to use CumulativeExportKindSelector() by default, as stated in open-telemetry/opentelemetry-specification#731. Adds WithExportKindSelector() option to the OTLP exporter to configure alternate policies. Using WithExportKindSelector(StatelessExportKindSelector()) results in the former PassThroughExporter behavior.

Add AggregationTemporality and IsMonotonic fields in OTLP exporter. Fixes #1288.

@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Oct 30, 2020
@codecov
Copy link

codecov bot commented Oct 30, 2020

Codecov Report

Merging #1296 (a564717) into master (3a06b39) will increase coverage by 0.0%.
The diff coverage is 88.3%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1296   +/-   ##
======================================
  Coverage    77.2%   77.3%           
======================================
  Files         122     122           
  Lines        5967    5978   +11     
======================================
+ Hits         4612    4622   +10     
- Misses       1106    1107    +1     
  Partials      249     249           
Impacted Files Coverage Δ
exporters/stdout/metric.go 76.5% <0.0%> (ø)
sdk/export/metric/exportkind_string.go 57.1% <66.6%> (-9.6%) ⬇️
exporters/otlp/internal/transform/metric.go 80.0% <88.2%> (+0.1%) ⬆️
exporters/metric/prometheus/prometheus.go 57.6% <100.0%> (ø)
exporters/otlp/options.go 54.5% <100.0%> (+4.5%) ⬆️
exporters/otlp/otlp.go 77.0% <100.0%> (+0.1%) ⬆️
sdk/export/metric/metric.go 97.3% <100.0%> (+0.5%) ⬆️
sdk/metric/processor/basic/basic.go 98.1% <100.0%> (-0.1%) ⬇️
sdk/metric/processor/processortest/test.go 89.5% <100.0%> (ø)

@jmacd jmacd changed the title Make OTLP AggregationTemporality configurable; add missing sum point fields Support configurable AggregationTemporality in exporters; add OTLP missing sum point temporality/monotonic fields Nov 3, 2020
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

👍

@jmacd
Copy link
Contributor Author

jmacd commented Nov 3, 2020

Someone privately asked if this PR should be split into smaller parts, as there are several things happening. I see these as all part of one bug fix, so I'm inclined to leave it the way it is, but just for the sake of discussion:

This could be sequenced as: (1) remove the PassThrough ExportKind, (2) change OTLP exporter to pass ExportKindSelector to where it's needed, (3) add the missing fields.

Note that (1) and (2) are independent, and that (3) depends on both. However, this assumes an approach that is not the only way forward. Instead of passing the ExportKindSelector into the Exporter where it re-calculates the export record's temporality (which requires it to be consistent), we could just add a field in the export record to say whether the record with its two timestamps is notionally DELTA or CUMULATIVE, for that's what we lack. We know the timestamps of the interval but we don't know whether to call the interval a DELTA or a CUMULATIVE.

I have another way to reason about this change of behavior. The last release uses OTLP v0.3, which lacked temporality fields. Although the existing code says that "passthrough" is the default export kind for OTLP, there were no fields in OTLP v0.3 to reflect temporality. We've renamed that concept "stateless" in this PR, and it's a critical concept IMO, but for months we've stated that the default would change to CUMULATIVE for OTLP and we couldn't do that until v0.5 support, so I see this "fix" as just part of finishing v0.5 support, not some elaborate new behavior about configurable aggregation temporality. (Aggregation temporality was always configurable, just not in a way the exporter can see.)

Therefore, @open-telemetry/go-approvers I'd like to review this as-is in the interest of a speedy release.

Copy link
Member

@krnowak krnowak 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.

exporters/otlp/otlp_metric_test.go Outdated Show resolved Hide resolved
exporters/otlp/otlp_metric_test.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor Author

jmacd commented Nov 10, 2020

I think this is ready to merge.

@MrAlias MrAlias merged commit f9984f2 into open-telemetry:master Nov 10, 2020
@jmacd jmacd deleted the jmacd/otlpfix branch November 11, 2020 19:22
@MrAlias MrAlias mentioned this pull request Nov 20, 2020
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
…ssing sum point temporality/monotonic fields (open-telemetry#1296)

* Restructure ExportKindSelector helpers; eliminate PassThroughExportKind; add StatelessExportKindSelector()

* WithExportKindSelector(); Additional testing

* Changelog update

* Test the new selectors

* From review feedback

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@pellared pellared added this to the untracked milestone Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exported OTLP metric Int/DoubleSum data points lack temporality / is_monotonic fields
6 participants