Skip to content

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

Closed
Yun-Ting wants to merge 31 commits intoopen-telemetry:mainfrom
Yun-Ting:yunl/RefactorPrometheusExporter
Closed

[Prometheus] Split up projects based on hosting mechanism.#3375
Yun-Ting wants to merge 31 commits intoopen-telemetry:mainfrom
Yun-Ting:yunl/RefactorPrometheusExporter

Conversation

@Yun-Ting
Copy link
Copy Markdown
Contributor

@Yun-Ting Yun-Ting commented Jun 16, 2022

Fixes #3057.

Changes

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 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. Please refer to readme for more details.

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

TODO:
Currently, OpenTelemetry.Exporter.Prometheus.AspNetCore.Tests, OpenTelemetry.Exporter.Prometheus.HttpListener.Tests, and OpenTelemetry.Exporter.Prometheus.Shared.Tests are green. I'll be working on fixing the CIs (not sure if I broke other tests with bad merge.)

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
Copy Markdown

codecov bot commented Jun 17, 2022

Codecov Report

Merging #3375 (dec7d9d) into main (f4dddbe) will decrease coverage by 0.04%.
The diff coverage is 93.54%.

❗ Current head dec7d9d differs from pull request most recent head be87d44. Consider uploading reports for the commit be87d44 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3375      +/-   ##
==========================================
- Coverage   86.25%   86.20%   -0.05%     
==========================================
  Files         258      272      +14     
  Lines        9269     9519     +250     
==========================================
+ Hits         7995     8206     +211     
- Misses       1274     1313      +39     
Impacted Files Coverage Δ
.../PrometheusExporterApplicationBuilderExtensions.cs 100.00% <ø> (ø)
...rometheusExporterEndpointRouteBuilderExtensions.cs 100.00% <ø> (ø)
...metheus.AspNetCore/PrometheusExporterMiddleware.cs 67.85% <ø> (ø)
...us/Implementation/PrometheusExporterEventSource.cs 27.77% <ø> (+11.11%) ⬆️
...elemetry.Exporter.Prometheus/PrometheusExporter.cs 100.00% <ø> (+9.09%) ⬆️
...y.Exporter.Prometheus/PrometheusExporterOptions.cs 57.14% <ø> (-27.07%) ⬇️
....Prometheus.HttpListener/PrometheusHttpListener.cs 82.35% <90.00%> (ø)
...heus.HttpListener/PrometheusHttpListenerOptions.cs 100.00% <100.00%> (ø)
...tp/Implementation/HttpHandlerDiagnosticListener.cs 69.31% <0.00%> (-3.86%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
... and 30 more

@Yun-Ting Yun-Ting marked this pull request as ready for review June 18, 2022 00:16
@Yun-Ting Yun-Ting requested a review from a team June 18, 2022 00:16
@cijothomas
Copy link
Copy Markdown
Member

@Yun-Ting Could you update the description with the final list of packages and their connections? It is not clear what would happend to the package OpenTelemetry.Exporter.Prometheus... Do we continue to publish it? Who is it meant for? Can one use it directly or its not meant for end user consumption?

Copy link
Copy Markdown
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Need to see description of the overall package architecture and dependencies, and the expected/proposed end user experience.

@Yun-Ting
Copy link
Copy Markdown
Contributor Author

Need to see description of the overall package architecture and dependencies, and the expected/proposed end user experience.

It depends on whether we think there will be use cases for extending directly on OpenTelemetry.Exporter.Prometheus project (not using the middleware or the listener.)
I'm currently leaning towards on not exposing the OpenTelemetry.Exporter.Prometheus itself as a nuget and in the OpenTelemetry.Exporter.Prometheus.AspNetCore, and OpenTelemetry.Exporter.Prometheus.HttpListener, referencing the shared files as links.
What is your take @cijothomas ?

@cijothomas
Copy link
Copy Markdown
Member

Need to see description of the overall package architecture and dependencies, and the expected/proposed end user experience.

It depends on whether we think there will be use cases for extending directly on OpenTelemetry.Exporter.Prometheus project (not using the middleware or the listener.) I'm currently leaning towards on not exposing the OpenTelemetry.Exporter.Prometheus itself as a nuget and in the OpenTelemetry.Exporter.Prometheus.AspNetCore, and OpenTelemetry.Exporter.Prometheus.HttpListener, referencing the shared files as links. What is your take @cijothomas ?

#3057 - the proposal from the issue is good.

{
private readonly PrometheusExporter exporter;
private readonly HttpListener httpListener = new();
private readonly System.Net.HttpListener httpListener = new();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
private readonly System.Net.HttpListener httpListener = new();
private readonly HttpListener httpListener = new();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will get this waring without using System.Net for it :(.
What are the suggested ways to not spelling out the entire namespace for System.Net.HttpListener while not changing the PrometheusHttpListener namespace?
image

@Yun-Ting
Copy link
Copy Markdown
Contributor Author

@Yun-Ting Could you update the description with the final list of packages and their connections? It is not clear what would happend to the package OpenTelemetry.Exporter.Prometheus... Do we continue to publish it? Who is it meant for? Can one use it directly or its not meant for end user consumption?

@cijothomas , I've updated the PR description to answer these questions. I don't think we'll need to publish OpenTelemetry.Exporter.Prometheus.Shared as a nuget package.
We will have 2 separate nuget packages, which are OpenTelemetry.Exporter.Prometheus.HttpListener and OpenTelemetry.Exporter.Prometheus.AspNetCore.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 6, 2022

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jul 6, 2022
{
public static MeterProviderBuilder AddPrometheusHttpListener(
this MeterProviderBuilder builder,
Action<PrometheusExporterOptions> configureExporterOptions = null,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where is PrometheusExporterOptions class defined? Why we need two separate options class?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PrometheusExporterOptions class in the OpenTelemetry.Exporter.Prometheus.Shared project and is meant to contain the fields that were shared by Listener and AspNetCore.


The `PrometheusExporter` can be configured using the `PrometheusExporterOptions`
properties. Refer to
[`TestPrometheusExporter.cs`](../../examples/Console/TestPrometheusExporter.cs)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this'd be incorrect, as the link takes you to a console example, where this readme.md is specifically targetting asp.net core apps

Copy link
Copy Markdown
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

See comments. Very close, but some refinements needed in the shared class usage, and public API

@Yun-Ting
Copy link
Copy Markdown
Contributor Author

Yun-Ting commented Jul 7, 2022

Closing this PR - please review the new #3430 instead. Thanks!

@Yun-Ting Yun-Ting closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Stale Issues and pull requests which have been flagged for closing due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prometheus: Split up projects based on hosting mechanism

4 participants