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

[Prometheus] Split up projects based on hosting mechanism. #3430

Merged

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Jul 6, 2022

Fixes #3057.

Changes

Addressing some comments in #3375.
There are some big commits got merged over the course of the development of PR3375 and I figured starting from the more current main line is easier than resolving merge conflicts.

The updated project setup is as follows:

  • OpenTelemetry.Exporter.Prometheus.Shared contains the PrometheusExporter, PrometheusExporterOptions and serialization methods for all the shared classes to both AspNetCore and HttpListener projects. This project is not expected to be consumed directly by the end users. It serves as a clean way to group shared code for the below 2 projects for semantic reasons. As a result, I've deleted the Implementation folder that was in OpenTelemetry.Exporter.Prometheus project, and moved out the shared files which was previously in the folder to be the immediate children of the project.

  • OpenTelemetry.Exporter.Prometheus.HttpListener contains the PrometheusHttpListener and its options class. It aims to provide end users a quick setup experience for using OTel metrics with Prometheus Exporter locally.

  • OpenTelemetry.Exporter.Prometheus.AspNetCore contains the middleware and its extensions for registering. This is for users trying to run PrometheusExporter in production environment.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #3430 (2f75b66) into main (2b1e75d) will increase coverage by 0.25%.
The diff coverage is 73.33%.

❗ Current head 2f75b66 differs from pull request most recent head 4f5449f. Consider uploading reports for the commit 4f5449f to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3430      +/-   ##
==========================================
+ Coverage   86.25%   86.50%   +0.25%     
==========================================
  Files         272      275       +3     
  Lines        9939     9977      +38     
==========================================
+ Hits         8573     8631      +58     
+ Misses       1366     1346      -20     
Impacted Files Coverage Δ
.../PrometheusExporterApplicationBuilderExtensions.cs 100.00% <ø> (ø)
...rometheusExporterEndpointRouteBuilderExtensions.cs 100.00% <ø> (ø)
...metheus.AspNetCore/PrometheusExporterMiddleware.cs 67.85% <ø> (ø)
...etheus.HttpListener/PrometheusCollectionManager.cs 78.04% <ø> (ø)
...heus.HttpListener/PrometheusExporterEventSource.cs 27.77% <ø> (ø)
...ometheus.HttpListener/PrometheusExporterOptions.cs 57.14% <ø> (ø)
...er.Prometheus.HttpListener/PrometheusSerializer.cs 78.21% <ø> (ø)
...Prometheus.HttpListener/PrometheusSerializerExt.cs 100.00% <ø> (ø)
...orterHttpListenerMeterProviderBuilderExtensions.cs 57.14% <57.14%> (ø)
....Prometheus.HttpListener/PrometheusHttpListener.cs 81.57% <87.50%> (ø)
... and 14 more

@Yun-Ting Yun-Ting changed the title [Not for merging] Split Prometheus [Prometheus] Split up projects based on hosting mechanism. Jul 7, 2022
@Yun-Ting Yun-Ting marked this pull request as ready for review July 7, 2022 22:47
@Yun-Ting Yun-Ting requested a review from a team as a code owner July 7, 2022 22:47
@@ -0,0 +1,6 @@
OpenTelemetry.Exporter.Prometheus.Shared.PrometheusExporterOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to run publicApi analyzer for the shared project. This might require adding some new condition in Common.prod.props. Currently, publicApi analyzer is run for every project under src/.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

LGTM. Left some minor suggestions.

Copy link
Member

@CodeBlanch CodeBlanch 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 nits but LGTM

/// <summary>
/// EventSource events emitted from the project.
/// </summary>
[EventSource(Name = "OpenTelemetry-Exporter-Prometheus")]
Copy link
Member

Choose a reason for hiding this comment

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

may need to be changed to more specific

[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.Tests" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Extensions.EventSource" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.Shared" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.AspNetCore" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("OpenTelemetry.Exporter.Prometheus.AspNetCore.Tests" + AssemblyInfo.PublicKey)]
Copy link
Member

Choose a reason for hiding this comment

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

questionable why aspnetcore.tests need special access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using OpenTelemetry.Metrics;

namespace OpenTelemetry.Exporter
namespace OpenTelemetry.Exporter.Prometheus
Copy link
Member

Choose a reason for hiding this comment

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

not sure ns need a change here? The class name already have Prometheus in it..

@Yun-Ting Yun-Ting force-pushed the yunl/RefactorPrometheusExporter2 branch from 2f75b66 to 6dfc90c Compare July 28, 2022 22:42
{
/// <summary>
/// <see cref="PrometheusExporter"/> options.
/// Prometheus exporter options.
Copy link
Member

Choose a reason for hiding this comment

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

the ns already has prometheus in it, so class name again having prometheus is bit looong.

@cijothomas
Copy link
Member

Merging to make progress. Rest of the comments can be addressed as follow ups.

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.

Prometheus: Split up projects based on hosting mechanism
7 participants