Add Prometheus Remote Write Exporter (3/6)

[shovnik]

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 (change logs will be updated when PR 7/7 is merged and the RW exporter is complete)
• Unit tests have been added
• Documentation has been updated
  cc- @AzfaarQureshi , @alolita

[ocelotl]

Please take a look at the comments and check what can be refactored from the test cases. 👍

[lzchen]

I'm wondering if we might get labels that are supposed to be unique but will be dropped due to removal of these characters. Is the alphanumeric + underscores limitation of the labels specific for Prometheus?

e.g.
testing and ${testing}

[shovnik]

The limitation is specific to the TimeSeries type https://prometheus.io/docs/concepts/data_model/ . You bring up a valid point. Since the non-alphanumeric characters are replaced with underscores: '$testing' and '_testing' will be considered the same label which would be an issue. Also, a label_name cannot start with two underscores. Would it be better to validate label names (raise Error) or add more complex sanitizing? I am not quite sure how to prevent duplicates when addressing label names that contain non-alphanumeric characters.

[lzchen]

If the limitation is Prometheus specific, I am okay with the current implementation. The only problem was if it were a limitation we imposed on the OT side, then Prometheus users would have unexpected behaviour. But if it's on the Prometheus side, then user's should EXPECT this behaviour.

I wonder in this case if we should DROP labels that defy the TimeSeries definition (and maybe log a warning), instead of only dropping the duplicate.

[shovnik]

In the rare case this occurs, I think whoever is misusing the exporter will value the label with the non-ascii characters as much as the valid one, so I suggest dropping the duplicate but logging a warning that after sanitizing, a duplicate label had to be dropped.

[lzchen]

Some comments.

[shovnik]

All comments have been addressed @lzchen.

[lzchen]

I don't think we want to return here? We just want to skip the record.

[shovnik]

I thought to skip the record, I would need to return both here and in the calling "export" method. I was planning on adding this to the export method:
if timeseries is None:
return

Is there a better way to skip the record from an inner method?

[shovnik]

I misunderstood it as skip the batch. I fixed this to only skip the record and log a warning. I will add an additional check to not send empty requests in the subsequent PR.

[lzchen]

Nice!