Add OTLP metric exporter #835

ocelotl · 2020-06-17T03:35:34Z

Fixes #797

codeboten

This looks pretty good! I haven't had a chance to review the proto for metrics yet to see if there's anything missing there. Change requested in the comments.

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/metrics_exporter/__init__.py

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py

ext/opentelemetry-ext-otlp/tests/test_otlp_metric_exporter.py

codeboten · 2020-06-17T16:37:23Z

Please add a changelog entry

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py

lzchen · 2020-06-22T16:03:17Z

Could you fix otcollector_example in the example docs as well to use the new exporters?

ocelotl · 2020-06-24T18:23:01Z

Could you fix otcollector_example in the example docs as well to use the new exporters?

Fixed

aabmass · 2020-07-06T18:24:38Z

I noticed there is a new release of opentelemetry-proto v0.4.0. Should we upgrade the proto code and then update this PR against the new version or would it be better as a separate PR?

aabmass

Nice, LGTM!

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/metrics_exporter/__init__.py

lzchen

Nice

lzchen

Nice!

docs/getting-started.rst

docs/getting_started/otlpcollector_example.py

aabmass · 2020-08-05T13:58:15Z

...ter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/metrics_exporter/__init__.py

+    if isinstance(instrument, (Counter, UpDownCounter)):
+        temporality = MetricDescriptor.Temporality.DELTA


Should this be based on stateful @lzchen?

If you create a stateful Counter, I believe the exported values would be cumulative since the start of the process?

# interval starts counter.add(5) # ... counter.add(5) # interval ends, export() passed 10 # next interval starts counter.add(10) # interval ends, exporter() passed 20

Yes temporality should be based off statefulness. Currently it is a feature of the batcher but this might be changed soon.

I believe OTLP is supposed to describe the metrics on the wire rather than the instruments that created them, so that whatever is receiving the OTLP can make sense of it. For instance, if the collector was receiving DELTAs and then was supposed to export them to prometheus (which doesn't accept deltas for a cumulative metric), it knows to convert the DELTAs to CUMULATIVEs before exporting to prometheus.

So in the example above, if the SDK sends 10, 20 to the collector over OTLP with DELTA, then the collector converts them to cumulatives 10, 30 which is sending incorrect values to prometheus because it was already converted to cumulative in the SDK. That's why I think we should choose based on stateful.

The metrics API spec doesn't mention "stateful" at all. I think that's why the table there says that Counter/UpDownCounter would be delta, because it is assuming the SDK just sends off the DELTA is sums for the interval (stateful=False)

OTEP #131 (was just merged) changes the default behavior of OTLP exporters to send CUMULATIVES for Counter/UpDownCounter and allows configuration. The python sdk's "stateful" seems like that configuration. Point being, I don't think it's imperative to follow the table in the metrics API spec.

I think we will need to revisit this again very soon since OTLP is changing quickly, so I wouldn't worry too much about it not being perfect yet.

aabmass · 2020-08-05T14:02:27Z

...ter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/metrics_exporter/__init__.py

+                        labels=string_key_values,
+                        value=view_data.aggregator.current,


What about start and end times https://github.com/open-telemetry/opentelemetry-proto/blob/e43e1abc40428a6ee98e3bfd79bec1dfa2ed18cd/opentelemetry/proto/metrics/v1/metrics.proto#L282-L286

// Int64DataPoint is a single data point in a timeseries that describes the time-varying // values of a int64 metric. message Int64DataPoint { // The set of labels that uniquely identify this timeseries. repeated opentelemetry.proto.common.v1.StringKeyValue labels = 1; // start_time_unix_nano is the time when the cumulative value was reset to zero. // This is used for Counter type only. For Gauge the value is not specified and // defaults to 0. // // The cumulative value is over the time interval (start_time_unix_nano, time_unix_nano]. // Value is UNIX Epoch time in nanoseconds since 00:00:00 UTC on 1 January 1970. // // Value of 0 indicates that the timestamp is unspecified. In that case the timestamp // may be decided by the backend. fixed64 start_time_unix_nano = 2; // time_unix_nano is the moment when this value was recorded. // Value is UNIX Epoch time in nanoseconds since 00:00:00 UTC on 1 January 1970. fixed64 time_unix_nano = 3; // value itself. int64 value = 4; }

@lzchen this is another place we could use the start/end time in the metric SDK 😄

I'm not sure what you mean by end time? If it's not defined in the proto we can't really change it can we?

@lzchen Sorry, updated my previous comment to include the whole message. Specifically this part:

The cumulative value is over the time interval (start_time_unix_nano, time_unix_nano].

SIG Meeting update: add a TODO and issue to start sending these. Changes are needed in the SDK to support this

aabmass · 2020-08-05T14:05:50Z

exporter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/exporter.py

+
+
+def _get_resource_data(
+    sdk_resource_instrumentation_library_data, resource_class, name


do we want type hints throughout some of these other functions?

...ter/opentelemetry-exporter-otlp/src/opentelemetry/exporter/otlp/metrics_exporter/__init__.py

aabmass

I agree we can merge this and iterate 🙂 is there an issue to track the outstanding things here?

codeboten · 2020-08-18T04:05:48Z

I captured the remaining open items in different issues.

ocelotl added metrics exporters ext labels Jun 17, 2020

ocelotl requested a review from a team June 17, 2020 03:35

ocelotl self-assigned this Jun 17, 2020

codeboten suggested changes Jun 17, 2020

View reviewed changes

ocelotl force-pushed the issue_797 branch 3 times, most recently from cfc36e3 to 2807d26 Compare June 18, 2020 18:48

ocelotl requested a review from codeboten June 19, 2020 14:41

lzchen reviewed Jun 22, 2020

View reviewed changes

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py Outdated Show resolved Hide resolved

lzchen mentioned this pull request Jun 22, 2020

Bug using CollectorMetricsExporter with Measure metric #704

Closed

ocelotl force-pushed the issue_797 branch from 2807d26 to a52921b Compare June 24, 2020 16:56

ocelotl requested a review from lzchen June 24, 2020 18:23

ocelotl force-pushed the issue_797 branch from 4fe97da to 64f291a Compare June 25, 2020 16:31

aabmass approved these changes Jul 6, 2020

View reviewed changes

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/exporter.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-otlp/src/opentelemetry/ext/otlp/metrics_exporter/__init__.py Outdated Show resolved Hide resolved

lzchen approved these changes Jul 7, 2020

View reviewed changes

codeboten mentioned this pull request Jul 17, 2020

OpenTelemetry collector integration #923

Closed

ocelotl force-pushed the issue_797 branch from 6774b5b to e7b56de Compare August 4, 2020 22:52

ocelotl requested review from lzchen and aabmass August 4, 2020 22:52

lzchen approved these changes Aug 5, 2020

View reviewed changes

aabmass reviewed Aug 5, 2020

View reviewed changes

aabmass self-requested a review August 5, 2020 14:12

ocelotl force-pushed the issue_797 branch 2 times, most recently from 26fdc1f to 1b4b1aa Compare August 10, 2020 20:59

ocelotl added 22 commits August 17, 2020 14:54

Add OTLP metric exporter

f8ac7c7

Break in smaller methods

7ea96a8

Updating

99288be

More updatin

2251818

Fix lint

641f253

Fix lint

38030b3

Add changelog entry

c8c62ac

Fix ordering for 35

632b1f5

Fix lint

48e8916

Fix docstring

3f34857

Updating example

1ac507e

Fix docs

93bf1ff

Fix lint

7ab7984

Fix logging message

02a788e

Move exporter to new location

522cf09

WIP

2cb0abb

All tests passing

b3711ba

Fix lint

44b9432

Fix docs

c693326

Update port

d06d654

Update docs

338cc53

Add fixes

f9df1e4

ocelotl force-pushed the issue_797 branch from 60600d0 to f9df1e4 Compare August 17, 2020 20:55

aabmass approved these changes Aug 17, 2020

View reviewed changes

This was referenced Aug 18, 2020

Revisit statefulness in OTLP MetricExporter #1007

Closed

Send start_time_unix_nano and time_unix_nano in OTLP MetricExporter #1008

Closed

Add type hints to OTLP exporter #1009

Closed

codeboten merged commit 452be59 into open-telemetry:master Aug 18, 2020

ocelotl deleted the issue_797 branch September 30, 2020 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OTLP metric exporter #835

Add OTLP metric exporter #835

ocelotl commented Jun 17, 2020 •

edited

Loading

codeboten left a comment

codeboten commented Jun 17, 2020

lzchen commented Jun 22, 2020

ocelotl commented Jun 24, 2020

aabmass commented Jul 6, 2020

aabmass left a comment

lzchen left a comment

lzchen left a comment

aabmass Aug 5, 2020 •

edited

Loading

lzchen Aug 11, 2020

aabmass Aug 13, 2020

aabmass Aug 5, 2020 •

edited

Loading

lzchen Aug 11, 2020

aabmass Aug 12, 2020

aabmass Aug 13, 2020

aabmass Aug 5, 2020

aabmass left a comment

codeboten commented Aug 18, 2020

		if isinstance(instrument, (Counter, UpDownCounter)):
		temporality = MetricDescriptor.Temporality.DELTA

		labels=string_key_values,
		value=view_data.aggregator.current,



		def _get_resource_data(
		sdk_resource_instrumentation_library_data, resource_class, name

Add OTLP metric exporter #835

Add OTLP metric exporter #835

Conversation

ocelotl commented Jun 17, 2020 • edited Loading

codeboten left a comment

Choose a reason for hiding this comment

codeboten commented Jun 17, 2020

lzchen commented Jun 22, 2020

ocelotl commented Jun 24, 2020

aabmass commented Jul 6, 2020

aabmass left a comment

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

aabmass Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

lzchen Aug 11, 2020

Choose a reason for hiding this comment

aabmass Aug 13, 2020

Choose a reason for hiding this comment

aabmass Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

lzchen Aug 11, 2020

Choose a reason for hiding this comment

aabmass Aug 12, 2020

Choose a reason for hiding this comment

aabmass Aug 13, 2020

Choose a reason for hiding this comment

aabmass Aug 5, 2020

Choose a reason for hiding this comment

aabmass left a comment

Choose a reason for hiding this comment

codeboten commented Aug 18, 2020

ocelotl commented Jun 17, 2020 •

edited

Loading

aabmass Aug 5, 2020 •

edited

Loading

aabmass Aug 5, 2020 •

edited

Loading