Prometheus Remote Write Exporter (6/6) by AzfaarQureshi · Pull Request #227 · open-telemetry/opentelemetry-python-contrib

AzfaarQureshi · 2020-12-02T00:10:29Z

Description

This is PR 2/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

👉 Part 6/6

Add README, Design Doc and other necessary documentation.
Add sample app running in dockerized environment

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Readme and documentation changes

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- @shovnik, @alolita

lzchen · 2020-12-11T15:45:06Z

+Supported Aggregators
+---------------------
+
+-  Sum


I think exposing the supported aggregators is great. Just wondering if we should actually just link to the metrics sdk definitions, so we don't have to always update this file when the sdk supports more aggregations.

Due to differences between the aggregator data model and Prometheus's TimeSeries definition, each aggregator is actually converted separately to theTimeSeries format. So even if the SDK adds more aggregators, this exporter might not be able to work with them unless a conversion method is added here :/

That's why I thought it would be worth explicitly listing out which aggregators are supported

It would be nice to include a short description of what how these aggregators will behave and how they will accumulate the values.

Thats a good idea, I added a link to the relevant section in the metric spec instead in case the behavior is subject to change

I thought the point of listing out the aggregators was because they are the ones that Prometheus supports? We should be linking to Prometheus documentation of supported aggregators and what they do, instead of linking to OpenTelemetry documentation.

We are exporting OTLP metrics to Prometheus remote write integrated back-ends in timeseries format so despite the name, we never actually touch Prometheus metrics any step along the way, we bypass the Prometheus format. The only link is that our timeseries are in the same format Prometheus metrics would be if they were exported to these back-ends. We thought users would be interested to know how exactly their OTLP aggregations would be once converted to timeseries since that is what they will see on the back-end. We could also link to the Prometheus documentation that guide us on the timeseries format, but I think the OTLP aggregation documentation is important to understand what the exporter did to a user's metrics.

as per our meeting today, I've added a table comparing the OTLP aggegator with its equivalent Prometheus data type

lzchen

Nice.

AzfaarQureshi · 2020-12-21T18:48:57Z

@lzchen @codeboten Hey, is this ready to be merged? 😃

alertedsnake · 2020-12-22T15:17:25Z

Out of curiosity, is there a reason that python-snappy is not listed as a requirement in setup.cfg?

shovnik · 2020-12-22T16:34:47Z

@alertedsnake Yes, for some reason, the python-snappy module depends on the snappy library which must separately installed with apt, rpm or brew as described here: https://github.com/andrix/python-snappy. This means adding the python-snappy module to the setup.cfg by itself fails due to missing its snappy.h include. We could not find a way to add a C++ dependency so we detailed how to install python-snappy correctly in the README instead. If you know of any workaround, please let us know.

alertedsnake · 2020-12-22T16:50:07Z

Yep, your docs are pretty clear but I still think adding the dependency in setup.py is the right choice - of course installation will fail if the C library is not installed, but that's a requirement for the upstream package, and I think the error printed if one is missing it is quite clear.

AzfaarQureshi · 2020-12-22T17:18:31Z

@alertedsnake makes sense! I've added it back

adding sample app adding examples readme fixing lint errors linting examples updating readme tls_config example excluding examples adding examples to exclude in all linters adding isort.cfg skip changing isort to path ignoring yml only adding it to excluded directories in pylintrc only adding exclude to directory removing readme.rst and adding explicit file names to ignore adding the rest of the files adding readme.rst back adding to ignore glob instead reverting back to ignore list converting README.md to README.rst

The check-links workflow fails on any PR that touches CHANGELOG.md because the full file is scanned and five historical entries contain broken URLs: - open-telemetry#1670 and open-telemetry#227 entries have a stray `]` inside the URL. - The open-telemetry#1033 entry is missing the `/` between the org and repo in the URL. - The `aws.ecs.*` spec link points to the old path in opentelemetry-specification; the content has since moved to the semantic-conventions repo. - The 1.12.0rc2-0.32b0 release tag does not exist on opentelemetry-python; drop the link, keep the heading text. Signed-off-by: Ali <[email protected]>

AzfaarQureshi requested review from a team, hectorhdzg and owais and removed request for a team December 2, 2020 00:10

AzfaarQureshi force-pushed the 5-prometheus-remote-write branch from 59b04ac to fe93e0d Compare December 2, 2020 00:21

AzfaarQureshi mentioned this pull request Dec 9, 2020

Prometheus Remote Write Exporter (5/6) #216

Merged

8 tasks

AzfaarQureshi force-pushed the 5-prometheus-remote-write branch 3 times, most recently from 4885d1b to 5358eb6 Compare December 9, 2020 19:15