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

PrometheusExporter namespace cleanup. #3530

Merged
merged 16 commits into from
Aug 5, 2022

Conversation

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #3530 (17e5fb9) into main (668b073) will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3530      +/-   ##
==========================================
+ Coverage   87.04%   87.20%   +0.16%     
==========================================
  Files         275      275              
  Lines        9959     9959              
==========================================
+ Hits         8669     8685      +16     
+ Misses       1290     1274      -16     
Impacted Files Coverage Δ
.../PrometheusExporterApplicationBuilderExtensions.cs 100.00% <ø> (ø)
...rometheusExporterEndpointRouteBuilderExtensions.cs 100.00% <ø> (ø)
...rometheusExporterMeterProviderBuilderExtensions.cs 100.00% <ø> (ø)
...metheus.AspNetCore/PrometheusExporterMiddleware.cs 67.85% <ø> (ø)
...Prometheus.AspNetCore/PrometheusExporterOptions.cs 50.00% <ø> (ø)
....Prometheus.HttpListener/PrometheusHttpListener.cs 78.66% <ø> (-4.00%) ⬇️
...theusHttpListenerMeterProviderBuilderExtensions.cs 75.00% <ø> (ø)
...heus.HttpListener/PrometheusHttpListenerOptions.cs 100.00% <ø> (ø)
...Listener/Internal/PrometheusExporterEventSource.cs 16.66% <0.00%> (-11.12%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 56.25% <0.00%> (-6.25%) ⬇️
... and 6 more

<Compile Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Prometheus.HttpListener\Internal\PrometheusExporterEventSource.cs" Link="Includes/PrometheusExporterEventSource.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Prometheus.HttpListener\Internal\PrometheusSerializer.cs" Link="Includes/PrometheusSerializer.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Prometheus.HttpListener\Internal\PrometheusSerializerExt.cs" Link="Includes/PrometheusSerializerExt.cs" />
<Compile Include="$(RepoRoot)\src\OpenTelemetry.Exporter.Prometheus.HttpListener\Implementation\PrometheusCollectionManager.cs" Link="Includes/PrometheusCollectionManager.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

Curious - why is this called "Implementation"?

Copy link
Member

@alanwest alanwest Aug 4, 2022

Choose a reason for hiding this comment

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

I vote for getting rid of all of our Implementation namespaces 😆.

Though, maybe we need to be careful making this decision across the board. For example, the Azure Function integration with OpenTelemetry depends on the Implementation namespace from our hosting extensions package https://github.com/Azure/azure-functions-host/pull/7253/files#diff-58a81e1c4618746e5a783499802b2a8eaa6a8119c4cb03c496dee52fd5b4b361R58.

That said, probably safe to remove the Implementation namespace for Prometheus stuff at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious - why is this called "Implementation"?

I was trying to follow the namespace naming conventions from the other exporters (Jaeger and Zipkin.)
But I agree that in this case PrometheusExporter.cs can not be really counted as implementation...

@alanwest, @cijothomas, @CodeBlanch, @reyang
May I ask for your suggestion on whether we should use OpenTelemetry.Exporter, OpenTelemetry.Exporter.Prometheus, or something like OpenTelemetry.Exporter.Prometheus.Internal as the namespace for the internal stuffs?

Copy link
Member

Choose a reason for hiding this comment

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

OpenTelemetry.Exporter.Prometheus would be good enough for all non-public stuff. The classes are non-public, so no need to again have a namespace .Internal/.Implementation.

Copy link
Member

Choose a reason for hiding this comment

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

My current thinking:

  1. Here we have two different concepts - namespace and folder name.
  2. Regarding folder name, if we chose to have a folder called "Implementation", we should be able to justify it - e.g. everything inside that folder is implementation, everything outside the folder is not the implementation (e.g. declaration, contract, API). Similarly, if there a folder called "Internal", everything inside that folder should be internal and everything else should not be internal.
  3. Regarding namespace - anything publicly exposed should have high bar; things that are not publicly exposed can have freedom to change as the project evolves, so I think the guiding principle is to avoid unnecessary confusion and extra overhead.
  4. Do we want folder names and namespaces to be aligned? In general it'll be good to have that, but I think it's a low order bit comparing to 2 and 3.

Copy link
Contributor Author

@Yun-Ting Yun-Ting Aug 4, 2022

Choose a reason for hiding this comment

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

Thank you very much Alan, Cijo and Reiley for the feedback!
I've reverted back the folder name for internal/shared files - use the term "Internal".
And updated to use OpenTelemetry.Exporter.Prometheus as the internal files namespace as suggested.

@Yun-Ting Yun-Ting marked this pull request as ready for review August 4, 2022 19:16
@Yun-Ting Yun-Ting requested a review from a team August 4, 2022 19:16
Copy link
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.

changes to public facing classes ns is good.

Its good to separately deal with internal classes namespaces.

@cijothomas cijothomas merged commit 666a0c1 into open-telemetry:main Aug 5, 2022
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.

4 participants