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

opentelemetry-exporter-prometheus: restore package #2321

Merged
merged 37 commits into from
Mar 4, 2022

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Dec 13, 2021

Description

The Prometheus exporter was moved into the metrics branch prior to releasing 1.0 for tracing. Now that the metrics API and SDK are further along, we can bring back the exporter and update it to use the new methods. The original code for this PR comes from https://github.com/open-telemetry/opentelemetry-python/tree/metrics/exporter/opentelemetry-exporter-prometheus

Fixes #2291

@codeboten codeboten force-pushed the codeboten/restore-prometheus branch 2 times, most recently from 1f636d7 to aad4633 Compare February 25, 2022 17:18
@codeboten codeboten marked this pull request as ready for review February 25, 2022 17:18
@codeboten codeboten requested a review from a team February 25, 2022 17:18
Alex Boten added 8 commits February 25, 2022 11:22
The Prometheus exporter was moved into the `metrics` branch prior to releasing 1.0 for tracing. Now that the metrics API and SDK are further along, we can bring back the exporter and update it to use the new methods. The original code for this PR comes from https://github.com/open-telemetry/opentelemetry-python/tree/metrics/exporter/opentelemetry-exporter-prometheus
@codeboten codeboten force-pushed the codeboten/restore-prometheus branch from 36f0a4f to 0ef40d5 Compare February 25, 2022 19:23
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Added some initial review comments

codeboten and others added 2 commits March 2, 2022 15:50
Co-authored-by: Diego Hurtado <ocelotl@users.noreply.github.com>
Co-authored-by: Nathaniel Ruiz Nowell <enruizno@uwaterloo.ca>
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

many moving parts here with latest changes. @codeboten would you mind adding some ascii diagram of the pipeline? that would really help me to get the better idea of whole thing.

One question I have is if there are multiple exporter configured ex (otlp, prometheus), is there a chance of discrepancies in metric output because of different collection cycles? each metric reader has separate state so this shouldn't be a problem.

@codeboten
Copy link
Contributor Author

many moving parts here with latest changes. @codeboten would you mind adding some ascii diagram of the pipeline? that would really help me to get the better idea of whole thing.

@srikanthccv Sure here's an attempt, please take a look:


 ------------------
| PullMetricReader | -- registers collect callback
 ------------------         |        
                            |
         --------------------------                          --------------------
        | PrometheusMetricExporter | <--- implementation of | PullMetricExporter |
         --------------------------                          --------------------
                    /\                                       
             ------------------
            | _CustomCollector | <---- registered with prometheus client library
             ------------------
                    /\
 -------------------------------------------
| ScrapeTarget (e.g localhost:8000/metrics) |
 -------------------------------------------

codeboten and others added 3 commits March 3, 2022 11:10
@codeboten
Copy link
Contributor Author

@srikanthccv @aabmass @ocelotl please take a look at the latest change, i've removed the added interfaces, which i think makes it a lot cleaner on the implementation side... still undecided if this makes things better for the users though.

@ocelotl ocelotl added the Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary label Mar 3, 2022
@lzchen lzchen merged commit b783d16 into open-telemetry:main Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approve Public API check This label shows that the public symbols added or changed in a PR are strictly necessary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Prometheus exporter into main
5 participants