[build] Use compiler constant to toggle experimental public apis #4735

CodeBlanch · 2023-08-02T19:06:11Z

What this PR does is move the Log Bridge artifacts (Logger, LoggerProvider, etc.) and Exemplar artifacts under a compiler constant/switch that will make them public or internal based on a build property. For pre-release builds (alpha/beta/rc) these things will be marked public and for stable releases they will be marked internal. The goal being to allow us to expose experimental things in order to gather feedback while reserving the right to make changes.

…epo.

codecov · 2023-08-02T19:21:38Z

Codecov Report

Merging #4735 (a112fc7) into main (a4fae4f) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4735      +/-   ##
==========================================
- Coverage   82.08%   82.07%   -0.02%     
==========================================
  Files         313      313              
  Lines       12784    12784              
==========================================
- Hits        10494    10492       -2     
- Misses       2290     2292       +2

Files Changed	Coverage Δ
...endencyInjectionLoggerProviderBuilderExtensions.cs	`100.00% <ø> (ø)`
...encyInjectionLoggingServiceCollectionExtensions.cs	`100.00% <ø> (ø)`
...c/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs	`3.53% <ø> (ø)`
src/OpenTelemetry.Api/Logs/LogRecordData.cs	`88.00% <ø> (ø)`
...nTelemetry.Api/Logs/LogRecordSeverityExtensions.cs	`0.00% <ø> (ø)`
src/OpenTelemetry.Api/Logs/Logger.cs	`100.00% <ø> (ø)`
src/OpenTelemetry.Api/Logs/LoggerProvider.cs	`66.66% <ø> (ø)`
...rc/OpenTelemetry.Api/Logs/LoggerProviderBuilder.cs	`100.00% <ø> (ø)`
...porter.Console/ConsoleExporterLoggingExtensions.cs	`0.00% <ø> (ø)`
...rter.InMemory/InMemoryExporterLoggingExtensions.cs	`66.66% <ø> (ø)`
... and 11 more

... and 2 files with indirect coverage changes

build/Common.props

CodeBlanch · 2023-08-03T00:11:41Z

build/Common.prod.props

+      <ExposeExperimentalFeatures Condition="'$(MinVerPreRelease)' != ''">true</ExposeExperimentalFeatures>
+      <ExposeExperimentalFeatures Condition="'$(MinVerPreRelease)' == ''">false</ExposeExperimentalFeatures>


@utpilla I'll need to verify once this is all done and building, but this change should automatically toggle ExposeExperimentalFeatures in the publish workflow.

You could probably also look at updating the publish-packages workflow by making use of the release event activity types:

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#release

.github/workflows/ci.yml

NuGet.config

sharwell · 2023-08-03T12:38:45Z

src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs

@@ -22,12 +22,21 @@
 using OpenTelemetry.Internal;
 using OpenTelemetry.Trace;

+#if EXPOSE_EXPERIMENTAL_FEATURES
+namespace OpenTelemetry.Logs.Experimental;


💡 I would discourage placing experimental features into a different namespace. This placement means it will be impossible to ship a new experimental feature by simply marking it public (which would be a non-breaking change for both source and binary consumers). By requiring a rename of the namespace, you end up with a source and binary breaking change which tends to produce churn that adds unnecessary ship risk to the process.

What you think about this feedback @alanwest?

By requiring a rename of the namespace, you end up with a source and binary breaking change which tends to produce churn that adds unnecessary ship risk to the process.

I definitely see some value in not placing the experimental features into their own namespace. It would make the process of stabilizing the experimental features seamless for customers (as they wouldn't need to update their namespaces).

I do not feel strongly that an Experimental namespace is the best way to address my underlying concern. It's just one idea and maybe a bad one at that. I'm open to other ideas. Since we talked about this offline, I'll summarize my concern and what I'm interested in brainstorming. Consider the following scenario

User begins using 1.6.0-beta

User starts using the experimental log API and/or exemplar APIs

Then 1.6.0 stable is release

User gets excited and upgrades to the stable release only to find their code does not compile anymore

There's a variety of things we can consider to ameliorate this pain

Include clear release notes

Immediately publish a 1.7.0-alpha.1 release on the heels of the 1.6.0 stable allowing users to stay on the latest code and continue using experimental APIs.

Though, since in my experience many folks do not read release notes, I was trying to consider an option that makes it more abundantly clear that a user is using an experimental feature.

In my opinion, I think the best way I've seen this done is to ship experimental stuff in a separate package altogether much like we did with the OTLP log exporter. This pattern has the additional benefit of aligning with what most other OTel language SDKs do today. Though, I suspect this pattern may be challenging to apply to the log and exemplar APIs, so if this is true, then I am supportive of the pattern this PR poses.

Though, at a minimum, I think we should make sure to do 1 and 2 above.

One possible starting place:
https://github.com/dotnet/designs/blob/main/accepted/2023/preview-apis/preview-apis.md#intended-usage
dotnet/runtime#85444
dotnet/roslyn#68702

I reverted the namespace and added this to the XML comments:

/// <remarks><b>WARNING</b>: This is an experimental API which might change or be removed in the future. Use at your own risk.</remarks>

When we consume the net8 bits we can also add ExperimentalAttribute into the mix.

…IMENTAL_FEATURES.

…features

utpilla · 2023-08-09T22:26:08Z

...derBuilderExtensions/Logs/OpenTelemetryDependencyInjectionLoggerProviderBuilderExtensions.cs

+    /// </summary>
+    /// <remarks>
+    /// <para><inheritdoc cref="AddInstrumentation{T}(LoggerProviderBuilder, T)" path="/remarks"/></para>
+    /// Note: The type specified by <typeparamref name="T"/> will be


Add the warning for this method as well?

It is there on line 41 via the inheritdoc. Easy to miss 😄 I did my best to not copy the warning text everywhere. It should be at most in 1 spot per assembly. In SDK there are 2, one for logs and one for exemplars. Because I figured it was a good idea to detach those two conceptual areas from each other.

utpilla · 2023-08-09T22:41:02Z

src/OpenTelemetry.Exporter.Console/OpenTelemetry.Exporter.Console.csproj

@@ -30,13 +30,13 @@
  </ItemGroup>

  <ItemGroup>
-    <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
+    <Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" Condition="'$(ExposeExperimentalFeatures)' == 'true'" />


nit: Create a new ItemGroup with this condition and place these four shared files under it.

utpilla · 2023-08-09T22:44:35Z

src/OpenTelemetry.Extensions.Hosting/AssemblyInfo.cs

@@ -19,6 +19,7 @@
 [assembly: InternalsVisibleTo("OpenTelemetry.Extensions.Hosting.Tests" + AssemblyInfo.PublicKey)]
 [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2" + AssemblyInfo.MoqPublicKey)]

+#if EXPOSE_EXPERIMENTAL_FEATURES


Why is this being wrapped in the experimental check?

When !EXPOSE_EXPERIMENTAL_FEATURES than OpenTelemetry.Extension.Hosting sees the internals of OpenTelemetry.Api and it also sees the internal static class AssemblyInfo from that same file.

It is essentially yet another InternalsVisibleTo problem 🤣

Check out this commit I pushed. I had the idea to use this new C# 11 file-local types feature to prevent these AssemblyInfo helper classes from flowing to other assemblies. Let me know what you think about that!

utpilla · 2023-08-09T22:47:28Z

src/OpenTelemetry.Instrumentation.AspNetCore/.publicApi/net6.0/PublicAPI.Unshipped.txt

@@ -27,3 +27,4 @@ static OpenTelemetry.Metrics.MeterProviderBuilderExtensions.AddAspNetCoreInstrum
 static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder) -> OpenTelemetry.Trace.TracerProviderBuilder
 static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, string name, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder
 static OpenTelemetry.Trace.TracerProviderBuilderExtensions.AddAspNetCoreInstrumentation(this OpenTelemetry.Trace.TracerProviderBuilder builder, System.Action<OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreInstrumentationOptions> configureAspNetCoreInstrumentationOptions) -> OpenTelemetry.Trace.TracerProviderBuilder
+virtual OpenTelemetry.Instrumentation.AspNetCore.AspNetCoreMetricsInstrumentationOptions.AspNetCoreMetricEnrichmentFunc.Invoke(string name, Microsoft.AspNetCore.Http.HttpContext context, ref System.Diagnostics.TagList tags) -> void


Is this being added because of the new public API analyzer?

Ya. The new beta seems to have improved delegate checking or something 🤣

The new beta seems to have improved delegate checking or something

Correct. The previous version listed delegates as simple types, without any indication of what the return value or parameter types were. The full delegate signature is captured by the implicitly-created Invoke method, which doesn't exist in source but now is included in the Public API files in order to catch breaking changes to these signatures.

utpilla

LGTM.

CodeBlanch · 2023-08-10T00:11:53Z

src/OpenTelemetry.Api.ProviderBuilderExtensions/AssemblyInfo.cs

+#pragma warning disable SA1400 // Access modifier should be declared
+file static class AssemblyInfo


@sharwell I couldn't find an order for the modifiers that would make SA1400/SA1206 and IDE0036 happy. I'm sure you are aware of this but I thought I would mention it.

DotNetAnalyzers/StyleCopAnalyzers#3588

Fixed in 1.2.0-beta.507 and newer.

Thanks @sharwell! Fixed it up here: 65637b0

CodeBlanch added 2 commits August 2, 2023 11:59

Attempting to make it easier to have experimental public api in the r…

25c2d25

…epo.

Add condition to ExposeExperimentalFeatures definition.

6ac362a

CodeBlanch added 5 commits August 2, 2023 12:25

Drop the extra shipped public api files.

01b9eae

Upgrade Microsoft.CodeAnalysis.PublicApiAnalyzers to latest beta.

64f0d85

Tweak.

3570de0

Added public api definitions that seem to be needed by the new beta.

ca84fcf

Use an experimental namespace.

e9ba384

utpilla reviewed Aug 2, 2023

View reviewed changes

build/Common.props Show resolved Hide resolved

utpilla reviewed Aug 2, 2023

View reviewed changes

build/Common.props Show resolved Hide resolved

Automatically set ExposeExperimentalFeatures in publish workflow.

29cc171

CodeBlanch commented Aug 3, 2023

View reviewed changes

utpilla reviewed Aug 3, 2023

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

sharwell reviewed Aug 3, 2023

View reviewed changes

NuGet.config Outdated Show resolved Hide resolved

sharwell reviewed Aug 3, 2023

View reviewed changes

CodeBlanch added 15 commits August 3, 2023 10:27

Code review.

4654de7

Revert experimental namespace. Add XML remarks to public versions.

3a8e562

Updates to OpenTelemetry.Api.ProviderBuilderExtensions logging api.

9f252c4

SDK updates moving logging and exemplar artifacts behind EXPOSE_EXPER…

a8db636

…IMENTAL_FEATURES.

CHANGELOG updates and fixes.

284d53f

Merge from main.

b5dc172

MD lint.

d2638cc

More MD lint.

e9a42d0

Update ConsoleExporter for experimental features.

dbe5984

Updated in-memory exporter for experimental features.

b66bdd7

CHANGELOG update for in-memory exporter.

b29d488

Update OTLP project for experimental features.

1339ce0

Updated hosting project for experimental features.

8b8d9b1

Fix for in-memory exporter public api files.

5cbf663

Fixes.

d7f53cc

CodeBlanch added 4 commits August 9, 2023 11:12

Merge remote-tracking branch 'upstream/main' into build-experimental-…

7780b98

…features

Lint fixes.

498467d

CI workflow fix.

8ef1e58

More lint fixes.

76d5a7c

CodeBlanch marked this pull request as ready for review August 9, 2023 18:47

CodeBlanch requested a review from a team August 9, 2023 18:47

utpilla reviewed Aug 9, 2023

View reviewed changes

utpilla approved these changes Aug 9, 2023

View reviewed changes

Use file scoping on AssemblyInfo helpers.

88d3273

CodeBlanch commented Aug 10, 2023

View reviewed changes

CodeBlanch added 2 commits August 9, 2023 17:20

Code review.

463e977

Update StyleCop and fix warnings using "file" or "required" modifiers.

65637b0

alanwest approved these changes Aug 10, 2023

View reviewed changes

Merge branch 'main' into build-experimental-features

a112fc7

reyang approved these changes Aug 10, 2023

View reviewed changes

CodeBlanch merged commit 3b4db6d into open-telemetry:main Aug 10, 2023

CodeBlanch deleted the build-experimental-features branch August 10, 2023 20:50

CodeBlanch mentioned this pull request Aug 8, 2023

[logs] Support OTel Logs Bridge API #4433

Open

53 tasks

utpilla mentioned this pull request Aug 11, 2023

OTLP Exporter: Conditionally compile experimental log features #4762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[build] Use compiler constant to toggle experimental public apis #4735

[build] Use compiler constant to toggle experimental public apis #4735

CodeBlanch commented Aug 2, 2023 •

edited

Loading

codecov bot commented Aug 2, 2023 •

edited

Loading

CodeBlanch Aug 3, 2023

utpilla Aug 3, 2023 •

edited

Loading

sharwell Aug 3, 2023 •

edited

Loading

CodeBlanch Aug 3, 2023

utpilla Aug 3, 2023

alanwest Aug 4, 2023

sharwell Aug 7, 2023 •

edited

Loading

CodeBlanch Aug 7, 2023

utpilla Aug 9, 2023 •

edited

Loading

CodeBlanch Aug 10, 2023

utpilla Aug 9, 2023 •

edited

Loading

CodeBlanch Aug 10, 2023

utpilla Aug 9, 2023

CodeBlanch Aug 10, 2023

utpilla Aug 9, 2023

CodeBlanch Aug 9, 2023

sharwell Aug 10, 2023

utpilla left a comment

CodeBlanch Aug 10, 2023

sharwell Aug 10, 2023

CodeBlanch Aug 10, 2023

		<ExposeExperimentalFeatures Condition="'$(MinVerPreRelease)' != ''">true</ExposeExperimentalFeatures>
		<ExposeExperimentalFeatures Condition="'$(MinVerPreRelease)' == ''">false</ExposeExperimentalFeatures>

		#pragma warning disable SA1400 // Access modifier should be declared
		file static class AssemblyInfo

[build] Use compiler constant to toggle experimental public apis #4735

[build] Use compiler constant to toggle experimental public apis #4735

Conversation

CodeBlanch commented Aug 2, 2023 • edited Loading

codecov bot commented Aug 2, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

utpilla Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

sharwell Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Aug 2, 2023 •

edited

Loading

codecov bot commented Aug 2, 2023 •

edited

Loading

utpilla Aug 3, 2023 •

edited

Loading

sharwell Aug 3, 2023 •

edited

Loading

sharwell Aug 7, 2023 •

edited

Loading

utpilla Aug 9, 2023 •

edited

Loading

utpilla Aug 9, 2023 •

edited

Loading