Add Prometheus Remote Write Exporter (4/6) #212

shovnik · 2020-11-24T20:55:54Z

Description

This is PR 3/6 of adding a Prometheus Remote Write Exporter in Python SDK and address Issue open-telemetry/opentelemetry-python#1302

Part 1/6

Adds class skeleton
Adds all function signatures

Part 2/6

Adds validation of exporter constructor commands
Add unit tests for validation

Part 3/6

Adds conversion methods from OTel metric types to Prometheus TimeSeries
Add unit tests for conversion

👉 Part 4/6

Adds methods to export metrics to Remote Write endpoint
Add unit tests for exporting

Part 5/6

Add integration tests using sample app and instance of Cortex

Part 6/6

Add README, Design Doc and other necessary documentation.

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added class TestValidation in test_prometheus_remote_write_exporter.py

Does This PR Require a Core Repo Change?

Yes. - Link to PR:
No.

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated
cc- @AzfaarQureshi , @alolita

ocelotl

Some test case result interpretations can be affected by the current implementation, please take a look.

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

...pentelemetry-exporter-prometheus-remote-write/tests/test_prometheus_remote_write_exporter.py

ocelotl

Well done! Just a request to remove unnecessary Mocks.

ocelotl

Great job!

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

lzchen · 2020-12-03T21:27:53Z

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

+                response.reason,
+                str(response.content),
+            )
+            return MetricsExportResult.FAILURE


Are we going to be handling retries? Timeouts?, etc?

The timeout can be adjusted, but if a request fails (timeout or otherwise), we just report failure instead of retrying to avoid requests getting clogged due to records being exported continuously. If we set a timeout, but then retried multiple times, the timeout would no longer limit the time spent attempting to export one set of records. Our reasoning was that missing data is not as bad as delayed data especially in cases of cumulative data where missing data can be interpolated or cases where data is exported on a very short interval. Does not retrying failed requests sound reasonable for this use case?

Depends on how robust we want the functionality of this exporter. We can handle it in a different PR in the future if is needed.

codeboten

A couple of questions I'd like to have answered before approving.

codeboten · 2020-12-04T16:57:32Z

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

@@ -16,6 +16,9 @@
 import re
 from typing import Dict, Sequence

+import requests


should dependencies be added for python-snappy and requests?

One issue that I am running into is that I cannot add python-snappy to the setup.cfg because it depends on the snappy library in C which I had to brew install to get. Is there a way to add a C dependency that a module requires? If not I might have to specify to first manually install snappy library and module before using exporter in README.

Excluded python-snappy from install-requires for now.

codeboten · 2020-12-04T16:59:19Z

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

+            data=message,
+            headers=headers,
+            auth=auth,
+            timeout=self.timeout,


would be good to add the default timeout value to the class documentation

Added, will also add to README

codeboten · 2020-12-04T17:01:58Z

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py

+                    self.tls_config["cert_file"],
+                    self.tls_config["key_file"],
+                )
+        response = requests.post(


should this be wrapped in a try block?

I did not consider that requests.post could throw exceptions in addition to returning HTTPErrors inside the response. Modified to use a try/except block which handles all request exceptions. (now raises HTTPErrors to be logged as well) This guarantees calling export() will never throw an exception.

lzchen · 2020-12-07T16:27:39Z

exporter/opentelemetry-exporter-prometheus-remote-write/setup.cfg

@@ -39,6 +39,8 @@ package_dir=
    =src
 packages=find_namespace:
 install_requires =
+    protobuf >= 3.13.0
+    requests == 2.25.0
    opentelemetry-api == 0.16.dev0


Could you update these dependencies to 0.17.dev0 as well as the current version? It's causing some build failures.

codeboten

Thanks for responding to all my comments.

shovnik requested a review from a team as a code owner November 24, 2020 20:55

shovnik requested review from owais and hectorhdzg and removed request for a team November 24, 2020 20:55

shovnik force-pushed the 3-prometheus-remote-write branch 4 times, most recently from 09fe5a2 to ac5f46f Compare November 24, 2020 22:17

shovnik changed the title ~~Prometheus Remote Write Exporter PR (4/6)~~ Add Prometheus Remote Write Exporter (4/6) Nov 24, 2020

AzfaarQureshi mentioned this pull request Nov 25, 2020

Prometheus Remote Write Exporter (5/6) #216

Merged

8 tasks

ocelotl requested changes Nov 26, 2020

View reviewed changes

AzfaarQureshi mentioned this pull request Nov 26, 2020

Add Prometheus Remote Write Exporter (2/6) #206

Merged

8 tasks

This was referenced Nov 26, 2020

Add Prometheus Remote Write Exporter (3/6) #207

Merged

Add Prometheus Remote Write Exporter (1/6) #180

Merged

shovnik force-pushed the 3-prometheus-remote-write branch 3 times, most recently from 9c932ae to f5499ce Compare November 27, 2020 00:53

ocelotl reviewed Nov 27, 2020

View reviewed changes

...pentelemetry-exporter-prometheus-remote-write/tests/test_prometheus_remote_write_exporter.py Outdated Show resolved Hide resolved

...pentelemetry-exporter-prometheus-remote-write/tests/test_prometheus_remote_write_exporter.py Outdated Show resolved Hide resolved

ocelotl requested changes Nov 27, 2020

View reviewed changes

AzfaarQureshi mentioned this pull request Nov 27, 2020

Prometheus Remote Write Exporter (6/6) open-o11y/opentelemetry-python-contrib#14

Closed

7 tasks

shovnik force-pushed the 3-prometheus-remote-write branch from f5499ce to dcf02ef Compare November 27, 2020 20:25

ocelotl approved these changes Nov 27, 2020

View reviewed changes

shovnik force-pushed the 3-prometheus-remote-write branch 4 times, most recently from b6251fb to f249cc9 Compare December 1, 2020 22:07

AzfaarQureshi mentioned this pull request Dec 2, 2020

Prometheus Remote Write Exporter (6/6) #227

Merged

7 tasks

shovnik force-pushed the 3-prometheus-remote-write branch 2 times, most recently from f199064 to 0dad10e Compare December 3, 2020 17:02

lzchen reviewed Dec 3, 2020

View reviewed changes

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py Show resolved Hide resolved

lzchen reviewed Dec 3, 2020

View reviewed changes

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Dec 3, 2020

View reviewed changes

...orter-prometheus-remote-write/src/opentelemetry/exporter/prometheus_remote_write/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Dec 3, 2020

View reviewed changes

shovnik force-pushed the 3-prometheus-remote-write branch 3 times, most recently from b1e6880 to 5da9eb2 Compare December 3, 2020 22:16

lzchen approved these changes Dec 4, 2020

View reviewed changes

codeboten suggested changes Dec 4, 2020

View reviewed changes

shovnik force-pushed the 3-prometheus-remote-write branch 3 times, most recently from 5395f9f to c79a4c9 Compare December 4, 2020 18:23

lzchen reviewed Dec 7, 2020

View reviewed changes

shovnik force-pushed the 3-prometheus-remote-write branch 2 times, most recently from 4847d6a to f9c341e Compare December 7, 2020 16:33

codeboten approved these changes Dec 7, 2020

View reviewed changes

shovnik force-pushed the 3-prometheus-remote-write branch 3 times, most recently from 7fea46a to 9c036e1 Compare December 7, 2020 16:44

Added exporter request methods

8e93659

shovnik force-pushed the 3-prometheus-remote-write branch from 9c036e1 to 8e93659 Compare December 8, 2020 17:14

lzchen merged commit b310ec1 into open-telemetry:master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prometheus Remote Write Exporter (4/6) #212

Add Prometheus Remote Write Exporter (4/6) #212

shovnik commented Nov 24, 2020 •

edited

Loading

ocelotl left a comment

ocelotl left a comment

ocelotl left a comment

lzchen Dec 3, 2020

shovnik Dec 3, 2020

lzchen Dec 4, 2020

codeboten left a comment

codeboten Dec 4, 2020

shovnik Dec 4, 2020

shovnik Dec 4, 2020

codeboten Dec 4, 2020

shovnik Dec 4, 2020

codeboten Dec 4, 2020

shovnik Dec 4, 2020

lzchen Dec 7, 2020

shovnik Dec 7, 2020

codeboten left a comment

Add Prometheus Remote Write Exporter (4/6) #212

Add Prometheus Remote Write Exporter (4/6) #212

Conversation

shovnik commented Nov 24, 2020 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

ocelotl left a comment

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

shovnik commented Nov 24, 2020 •

edited

Loading