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

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

Merged

Conversation

shovnik
Copy link
Contributor

@shovnik shovnik commented Nov 12, 2020

Description

This is PR 1/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 (Full documentation in final exporter PR)
    cc- @AzfaarQureshi , @alolita

@shovnik shovnik requested a review from a team as a code owner November 12, 2020 20:35
@shovnik shovnik requested review from codeboten and ocelotl and removed request for a team November 12, 2020 20:35
@shovnik shovnik force-pushed the SetupPrometheusRWExporter branch 20 times, most recently from 0c4ba24 to 31b910d Compare November 16, 2020 19:12
@shovnik shovnik changed the title Setup Prometheus Remote Write Exporter (WIP) Setup Prometheus Remote Write Exporter PR 1/7 Nov 19, 2020
@shovnik shovnik changed the title Setup Prometheus Remote Write Exporter PR 1/7 Setup Prometheus Remote Write Exporter PR 1/7 (WIP) Nov 19, 2020
@shovnik shovnik force-pushed the SetupPrometheusRWExporter branch 4 times, most recently from d0cab05 to a8c4ffc Compare November 20, 2020 00:06
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.

A few minor changes needed, would be good to add some info regarding the source of the protos for prometheus as well.

References
----------

* `Datadog <https://prometheus.io/>`_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `Datadog <https://prometheus.io/>`_
* `Prometheus <https://prometheus.io/>`_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

OpenTelemetry Prometheus Remote Write Exporter
==============================================

This library allows exporting metric data to `Prometheus Write Integrated Backends
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say Prometheus Remote Write?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

long_description_content_type = text/x-rst
author = OpenTelemetry Authors
author_email = cncf-opentelemetry-contributors@lists.cncf.io
url = https://github.com/open-telemetry/opentelemetry-python/exporter/opentelemetry-exporter-datadog
Copy link
Contributor

Choose a reason for hiding this comment

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

url needs updating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

=src
packages=find_namespace:
install_requires =
ddtrace>=0.34.0
Copy link
Contributor

Choose a reason for hiding this comment

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

is ddtrace necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is a remnant from the datadog exporter I used as template, removed.

@@ -0,0 +1,3 @@
# Changelog

## Unreleased
Copy link
Contributor

Choose a reason for hiding this comment

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

I would include "initial release" and the PRs associated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added Initial Release in CHANGELOG and will update it with each subsequent PR.

@lzchen
Copy link
Contributor

lzchen commented Nov 24, 2020

Did we decide to create this in the contrib repo? I thought we wanted to keep this in the same place as the prometheus exporter to avoid confusion?

@shovnik
Copy link
Contributor Author

shovnik commented Nov 24, 2020

We were not sure where the exporter would reside so thought we would make the PRs on the contrib repo for now to get the reviews going. Once the standard location for prometheus exporters is decided, we can move one or the other.

@shovnik
Copy link
Contributor Author

shovnik commented Nov 24, 2020

I addressed all the requested changes @lzchen @codeboten

@shovnik shovnik changed the title Prometheus Remote Write Exporter PR (1/7) Prometheus Remote Write Exporter PR (1/6) Nov 24, 2020
@shovnik shovnik changed the title Prometheus Remote Write Exporter PR (1/6) Add Prometheus Remote Write Exporter (1/6) Nov 24, 2020
@toumorokoshi
Copy link
Member

@lzchen @codeboten from a high level, how do you want to handle reviewing this six-parter?

It may be more efficient for the same people to review all PRs. happy to volunteer.

Currently I'm not too inclined to review 3/6 which I was assigned, since I have to go read 1 and 2 first anyway.

@@ -0,0 +1,82 @@
// Copyright 2016 Prometheus Team
Copy link
Member

Choose a reason for hiding this comment

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

it looks like these are copied from the prometheus library itself. Probably fine.

Are there any thoughts around what the update story would look like? just copy the protos over I suppose.

Copy link
Contributor

@AzfaarQureshi AzfaarQureshi Nov 25, 2020

Choose a reason for hiding this comment

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

just copy the protos over I suppose.

Yup! thats the plan currently

@lzchen
Copy link
Contributor

lzchen commented Nov 25, 2020

@toumorokoshi

I'm going to be reviewing all 6. Others are more than welcome to chime in as well.


## Unreleased

## Initial Release
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
this change can just be under the ## Unreleased section, when the release is done, the correct version tag title will be applied to the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased and updated CHANGELOG to use Unreleased

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.

Changes look good, holding off on merging until we get this release out to prevent releasing the prometheus package before the other PRs are merged

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.

Please update the branch and we can merge!

@shovnik shovnik force-pushed the SetupPrometheusRWExporter branch 2 times, most recently from 59b0634 to 83e15ea Compare November 27, 2020 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants