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 Exporter: Step 0 - Update MetricsExporter Interface #240

Merged
merged 11 commits into from
Aug 21, 2020

Conversation

erichsueh3
Copy link
Contributor

This PR implements headers in the sdk/metrics folder that lays out the groundwork for the Prometheus Exporter project. The Metrics interface, exporter.h, is the interface which the PrometheusExporter class is derived from. The ReturnCode class, return_code.h, lays out the possible return values returned when calling Export() from the PrometheusExporter class.

This PR is the first in a series of PRs that aims to add a full implementation of a Prometheus Exporter within the OpenTelemetry C++ Repository.

Note to reviewers: Please keep in mind that there are other PRs (yet to be filed) related to this one so codecov tests may not fully pass.

@erichsueh3 erichsueh3 requested a review from a team August 4, 2020 20:26
…hutdown() function not in Metrics specification
@erichsueh3 erichsueh3 changed the title Prometheus Exporter - MetricsExporter Interface and ReturnCode class Prometheus Exporter - MetricsExporter Interface Aug 5, 2020
@erichsueh3
Copy link
Contributor Author

To align our Metrics Exporter Interface with the previously existing Metrics Exporter Interface, we have removed record_code.h and added those codes to ExportResult in the previously existing exporter.h. Additionally, since there is no Shutdown function in the Metrics specification, we have removed that from our interface and aligned it with the existing interface.

@erichsueh3 erichsueh3 marked this pull request as draft August 18, 2020 01:07
@erichsueh3 erichsueh3 changed the title Prometheus Exporter - MetricsExporter Interface Add Prometheus Exporter: Step 0 - Update MetricsExporter Interface Aug 18, 2020
@erichsueh3 erichsueh3 marked this pull request as ready for review August 18, 2020 18:01
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Aug 19, 2020
@reyang
Copy link
Member

reyang commented Aug 20, 2020

@erichsueh3 please rebase and resolve the conflicts. Thanks.

Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I have one comment, but this looks good overall.

Comment on lines 69 to 73
/**
* In the Metrics specification, there is no Shutdown function required for exporters
* The Shutdown function can be implemented within exporters that wish to have one,
* but will not be enforced through this header
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't make sense and we should remove it.

The MetricsExporter is passed on to the Controller as a std::unique_ptr (see here). With that design, the user doesn't have the possibility to call any Shutdown method on the MetricsExporter, only the Controller could do so (but cannot if the interface doesn't have a Shutdown method).

I recommend removing this comment for now. We can extend the exporter interface by an optional shutdown function later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is originally included within the current exporter.h file, but I agree; will remove this comment for now.

@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #240 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #240   +/-   ##
=======================================
  Coverage   93.94%   93.94%           
=======================================
  Files         146      146           
  Lines        6538     6538           
=======================================
  Hits         6142     6142           
  Misses        396      396           
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/metrics/exporter.h 100.00% <ø> (ø)

@erichsueh3
Copy link
Contributor Author

@erichsueh3 please rebase and resolve the conflicts. Thanks.

@reyang rebased and resolved the conflicts, looks like all the checks are passing as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants