-
Notifications
You must be signed in to change notification settings - Fork 626
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
Prometheus Remote Write Exporter (6/6) #227
Prometheus Remote Write Exporter (6/6) #227
Conversation
59b04ac
to
fe93e0d
Compare
4885d1b
to
5358eb6
Compare
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
Supported Aggregators | ||
--------------------- | ||
|
||
- Sum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to include a short description of what how these aggregators will behave and how they will accumulate the values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per our meeting today, I've added a table comparing the OTLP aggegator with its equivalent Prometheus data type
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
e8a98c6
to
46a5169
Compare
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
exporter/opentelemetry-exporter-prometheus-remote-write/README.rst
Outdated
Show resolved
Hide resolved
4cc438a
to
2d5e7c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
@lzchen @codeboten Hey, is this ready to be merged? 😃 |
Out of curiosity, is there a reason that |
@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 |
Yep, your docs are pretty clear but I still think adding the dependency in |
@alertedsnake makes sense! I've added it back |
26c9059
to
73b012c
Compare
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
73b012c
to
4ed2a18
Compare
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
Part 2/6
Part 3/6
Part 4/6
Part 5/6
👉 Part 6/6
Type of change
How Has This Been Tested?
Readme and documentation changes
Does This PR Require a Core Repo Change?
Checklist:
cc- @shovnik, @alolita