Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics #1577

huyan0 · 2020-08-18T01:56:34Z

Description: This PR adds implementation for conversion from Int64, Monotonic Int64, Double, and Monotonic Double metrics to Prometheus TimeSeries, as well as the implementation for sending HTTP request to backend.

cc: @huyan0 @alolita @jmacd @bogdandrutu @annanay25 @gouthamve @open-telemetry/collector-approvers @open-telemetry/collector-maintainers

bogdandrutu

Can this wait 3-4 days? Want to first move the repo to the new protos

huyan0 · 2020-08-18T02:00:39Z

Can this wait 3-4 days? Want to first move the repo to the new protos

Yeah if its what make sense to do. I will close this for now

bogdandrutu · 2020-08-18T03:16:07Z

Don't have to close. I can still review but will merge later

huyan0 · 2020-08-18T03:48:40Z

Don't have to close. I can still review but will merge later

Yeah that would be great! I will tidy it up and reopen it tomorrow. Thanks.

huyan0 · 2020-08-18T12:50:00Z

Apologies for the large PR. I want to make sure tests are together with corresponding implementations. The new implementation is ~200 lines, and the rest are tests.

codecov · 2020-08-18T12:54:44Z

Codecov Report

Merging #1577 into master will decrease coverage by 0.03%.
The diff coverage is 89.89%.

@@            Coverage Diff             @@
##           master    #1577      +/-   ##
==========================================
- Coverage   92.25%   92.21%   -0.04%     
==========================================
  Files         262      262              
  Lines       18667    18765      +98     
==========================================
+ Hits        17221    17305      +84     
- Misses       1037     1044       +7     
- Partials      409      416       +7

Impacted Files	Coverage Δ
exporter/prometheusremotewriteexporter/exporter.go	`90.56% <88.63%> (-9.44%)`	⬇️
exporter/prometheusremotewriteexporter/helper.go	`100.00% <100.00%> (ø)`
receiver/otlpreceiver/otlp.go	`91.66% <0.00%> (-2.78%)`	⬇️
translator/internaldata/resource_to_oc.go	`87.03% <0.00%> (-1.86%)`	⬇️

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 42dd4c9...d1db707. Read the comment docs.

alolita · 2020-08-18T16:34:46Z

Hi @bogdandrutu Great to see your real-time changes for OTLP v2. I have a concern about the timing of when these code changes for the Collector and interface to exporter will be stable. Our interns @huyan0 @ercl are completing their internships by first week of September. We would like to make the necessary code changes and tests in the Prometheus remote write exporter we're developing before we can file PRs to handle OTLP v2 changes.

Is there a design doc or GitHub tag tracking all design changes you're making in the Collector for handling the updated OTLP definition? We're looking at opentelemetry-proto to understand the latest definition of the OTLP.

Thanks again for making these changes to enable OTLP to be the default data definition for the Collector.

cc: @jmacd @huyan0 @ercl

tigrannajaryan · 2020-08-18T17:24:12Z

Hi @bogdandrutu Great to see your real-time changes for OTLP v2.

@alolita What's OTLP v2? I thought we are still on v1?

huyan0 · 2020-08-18T17:28:37Z

Hi @bogdandrutu Great to see your real-time changes for OTLP v2.

@alolita What's OTLP v2? I thought we are still on v1?

I think it is referring to the new OTLP proto that Bogdan is moving this repository to. Would the new proto be associated with a version?

huyan0 · 2020-08-25T14:38:01Z

Hello @bogdandrutu, I noticed there were some updates made to OTLP in proto. Should I start refactoring my code according to those changes?

jmacd

@bogdandrutu I vote to merge this.

exporter/prometheusremotewriteexporter/exporter.go

jmacd · 2020-08-26T05:11:01Z

exporter/prometheusremotewriteexporter/exporter.go

+		return err
+	}
+
+	if httpResp.StatusCode/100 != 2 {


nit: I noticed the OTel-Go exporter tests for 200 (http.StatusOK) exactly. Do you think the other values really matter? The other value I'd maybe expect is 202. Do you think the other 200 codes matter?

I am not sure about other code. The reference here is the Prometheus remote write implementation:https://github.com/prometheus/prometheus/blob/2c4a4548a8382f7c8966dbfda37f34c43151e316/storage/remote/client.go#L187

alolita · 2020-08-26T07:15:32Z

Hi @bogdandrutu @jmacd Requesting your review for @huyan0 's PR since his internship ends in 7 days. Please merge this PR as soon as possible.

Also please provide an update on whether we should complete OTLP v1 support or wait for your code for OTLP v2 if you think it will land this week. 🙏

huyan0 · 2020-08-26T22:11:44Z

Per discussion with @bogdandrutu and @markcartertm in Collector SIG on August 26, the OTLP metrics definition is still being discussed, thus is doesn't make perfect sense to review this PR now. If no further progress on OTLP is made early next week, this PR will be reviewed. If not, I might not be able to work on this PR due to internship timeline.

exporter/prometheusremotewriteexporter/exporter.go

bogdandrutu · 2020-08-27T02:42:15Z

exporter/prometheusremotewriteexporter/exporter.go

+				Timestamp: convertTimeStamp(pt.TimeUnixNano),
+			}
+
+			addSample(tsMap, sample, labels, metric.GetMetricDescriptor().GetType())


I did not see that addSample treats "MONOTONIC_" as counters. Am I missing this?

Yes, I am treating MONOTONI_ and non-monotonic the same way and assuming current value is already calculated. Is this what you are referring to? The only other different thing I did for MONOTONIC_ is to add a _total suffix in its name. https://github.com/open-o11y/opentelemetry-collector/blob/166624cad9f682d2982cea442f5b10e5e1b92ed2/exporter/prometheusremotewriteexporter/helper.go#L178

The Prometheus remote write protocol doesn't make this distinction. (I'm not sure how this is handled in a complete system.)

https://www.robustperception.io/how-does-a-prometheus-counter-work. Here is a article explaining how Prometheus server handles a counter value reset. Counters are allowed to be reset to 0, so there's no need to do anything special here to handle it.

bogdandrutu · 2020-08-27T02:44:42Z

exporter/prometheusremotewriteexporter/exporter.go

+				Timestamp: convertTimeStamp(pt.TimeUnixNano),
+			}
+
+			addSample(tsMap, sample, labels, metric.GetMetricDescriptor().GetType())


Same, you seem to ignore the MONOTONIC part

Same as #1577 (comment)

bogdandrutu · 2020-08-27T02:46:23Z

@huyan0 started to review, if the proto changes land first you will rebase otherwise will refactor after.

bogdandrutu · 2020-08-27T02:46:57Z

@jmacd reviewing and provided feedback (hopefully useful feedback).

jmacd · 2020-08-27T20:00:01Z

This looks like it could / should merge. Let's discuss in today's Metric SIG meeting.

bogdandrutu

Need to replace all the imports with dataold instead of consumer/pdata or internal/data

…te Write Exporter add prometheus remote write exporter address lint issues improve test coverage fix lint error add prometheus remote write exporter to default components change metric name label add conversion from ns to ms rename tests add attribute to label functionality in exporter.go format code resolve conflicts with master switch to fmt.Errorf add conversion from Int64 and Double OTLP metrics add prometheus remote write exporter address lint issues improve test coverage fix lint error add prometheus remote write exporter to default components change metric name label add conversion from ns to ms rename tests add attribute to label functionality in exporter.go format code resolve conflicts with master add check for nil change data format to dataold

huyan0 · 2020-08-28T13:18:20Z

I have changed:

internal/data -> internal/dataold
internal/testdata -> internal/testdataold
opentelemetry-proto-gen/metrics/v1 -> opentelemetry-proto-gen/metrics/v1old 
pdatautil.MetricsToInternal -> pdatautil.MetricsToOldInternalMetrics

Thanks.

* Fix Windows build of Jaeger tests The Jaeger tests use the low-level syscall package. The Windows specific function called in that package has a different function signature than the unix version. Add a windows specific file using the build flags to isolate this OS specific functionality. * Add changes to changelog * Blind succeed to account for unimplemented functionality on Windows

…y#1577) Bumps [boto3](https://github.com/boto/boto3) from 1.23.2 to 1.23.3. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.23.2...1.23.3) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

huyan0 requested review from bogdandrutu, dmitryax, james-bebbington, nilebox, owais, pjanotti and tigrannajaryan as code owners August 18, 2020 01:56

bogdandrutu reviewed Aug 18, 2020

View reviewed changes

huyan0 closed this Aug 18, 2020

huyan0 reopened this Aug 18, 2020

huyan0 changed the title ~~Scalar exporter~~ Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics Aug 18, 2020

huyan0 marked this pull request as draft August 18, 2020 13:08

huyan0 force-pushed the scalar_exporter branch from 73f43af to 7418ab6 Compare August 18, 2020 13:37

huyan0 marked this pull request as ready for review August 18, 2020 14:04

huyan0 requested a review from bogdandrutu August 18, 2020 16:03

jrcamp assigned bogdandrutu Aug 19, 2020

huyan0 force-pushed the scalar_exporter branch from 3449935 to 8976d44 Compare August 20, 2020 19:09

jmacd approved these changes Aug 26, 2020

View reviewed changes

huyan0 force-pushed the scalar_exporter branch from 8976d44 to 89a6e31 Compare August 26, 2020 15:50

huyan0 mentioned this pull request Aug 27, 2020

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for Histogram OTLP metrics #1643

Merged

bogdandrutu reviewed Aug 27, 2020

View reviewed changes

huyan0 mentioned this pull request Aug 27, 2020

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for Summary OTLP metrics #1649

Closed

huyan0 force-pushed the scalar_exporter branch from 0e302f9 to 24f4db3 Compare August 28, 2020 00:43

bogdandrutu approved these changes Aug 28, 2020

View reviewed changes

huyan0 force-pushed the scalar_exporter branch from 24f4db3 to d1db707 Compare August 28, 2020 13:18

bogdandrutu merged commit 03faf67 into open-telemetry:master Aug 28, 2020

alolita mentioned this pull request Oct 26, 2020

Added OTel contribution details for Alolita Sharma open-telemetry/community#547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics #1577

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics #1577

huyan0 commented Aug 18, 2020 •

edited

Loading

bogdandrutu left a comment

huyan0 commented Aug 18, 2020 •

edited

Loading

bogdandrutu commented Aug 18, 2020

huyan0 commented Aug 18, 2020

huyan0 commented Aug 18, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

alolita commented Aug 18, 2020

tigrannajaryan commented Aug 18, 2020

huyan0 commented Aug 18, 2020 •

edited

Loading

huyan0 commented Aug 25, 2020

jmacd left a comment

jmacd Aug 26, 2020

huyan0 Aug 26, 2020

alolita commented Aug 26, 2020

huyan0 commented Aug 26, 2020

bogdandrutu Aug 27, 2020 •

edited

Loading

huyan0 Aug 27, 2020

jmacd Aug 27, 2020

huyan0 Aug 27, 2020 •

edited

Loading

bogdandrutu Aug 27, 2020 •

edited

Loading

jmacd Aug 27, 2020

bogdandrutu commented Aug 27, 2020

bogdandrutu commented Aug 27, 2020

jmacd commented Aug 27, 2020

bogdandrutu left a comment

huyan0 commented Aug 28, 2020 •

edited

Loading

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics #1577

Add Prometheus Remote Write Exporter supporting Cortex - conversion and export for scalar OTLP metrics #1577

Conversation

huyan0 commented Aug 18, 2020 • edited Loading

bogdandrutu left a comment

Choose a reason for hiding this comment

huyan0 commented Aug 18, 2020 • edited Loading

bogdandrutu commented Aug 18, 2020

huyan0 commented Aug 18, 2020

huyan0 commented Aug 18, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

alolita commented Aug 18, 2020

tigrannajaryan commented Aug 18, 2020

huyan0 commented Aug 18, 2020 • edited Loading

huyan0 commented Aug 25, 2020

jmacd left a comment

Choose a reason for hiding this comment

jmacd Aug 26, 2020

Choose a reason for hiding this comment

huyan0 Aug 26, 2020

Choose a reason for hiding this comment

alolita commented Aug 26, 2020

huyan0 commented Aug 26, 2020

bogdandrutu Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

huyan0 Aug 27, 2020

Choose a reason for hiding this comment

jmacd Aug 27, 2020

Choose a reason for hiding this comment

huyan0 Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

bogdandrutu Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

jmacd Aug 27, 2020

Choose a reason for hiding this comment

bogdandrutu commented Aug 27, 2020

bogdandrutu commented Aug 27, 2020

jmacd commented Aug 27, 2020

bogdandrutu left a comment

Choose a reason for hiding this comment

huyan0 commented Aug 28, 2020 • edited Loading

huyan0 commented Aug 18, 2020 •

edited

Loading

huyan0 commented Aug 18, 2020 •

edited

Loading

codecov bot commented Aug 18, 2020 •

edited

Loading

huyan0 commented Aug 18, 2020 •

edited

Loading

bogdandrutu Aug 27, 2020 •

edited

Loading

huyan0 Aug 27, 2020 •

edited

Loading

bogdandrutu Aug 27, 2020 •

edited

Loading

huyan0 commented Aug 28, 2020 •

edited

Loading