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

[api-logs] Define OTEL1000 & OTEL1001 diagnostics and decorate APIs #4963

Merged
merged 22 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shared", "Shared", "{A49299
ProjectSection(SolutionItems) = preProject
src\Shared\ActivityHelperExtensions.cs = src\Shared\ActivityHelperExtensions.cs
src\Shared\ActivityInstrumentationHelper.cs = src\Shared\ActivityInstrumentationHelper.cs
src\Shared\DiagnosticDefinitions.cs = src\Shared\DiagnosticDefinitions.cs
src\Shared\ExceptionExtensions.cs = src\Shared\ExceptionExtensions.cs
src\Shared\Guard.cs = src\Shared\Guard.cs
src\Shared\HttpSemanticConventionHelper.cs = src\Shared\HttpSemanticConventionHelper.cs
Expand Down Expand Up @@ -318,6 +319,12 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "getting-started-aspnetcore"
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "links-creation", "docs\trace\links-creation-with-new-activities\links-creation.csproj", "{B4856711-6D4C-4246-A686-49458D4C1301}"
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "diagnostics", "diagnostics", "{52AF6D7D-9E66-4234-9A2C-5D16C6F22B40}"
ProjectSection(SolutionItems) = preProject
docs\diagnostics\OTEL1000.md = docs\diagnostics\OTEL1000.md
docs\diagnostics\OTEL1001.md = docs\diagnostics\OTEL1001.md
EndProjectSection
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Debug|Any CPU = Debug|Any CPU
Expand Down Expand Up @@ -645,6 +652,7 @@ Global
{1C459B5B-C702-46FF-BF1A-EE795E420FFA} = {A49299FB-C5CD-4E0E-B7E1-B7867BBD67CC}
{99B4D965-8782-4694-8DFA-B7A3630CEF60} = {3862190B-E2C5-418E-AFDC-DB281FB5C705}
{B4856711-6D4C-4246-A686-49458D4C1301} = {5B7FB835-3FFF-4BC2-99C5-A5B5FAE3C818}
{52AF6D7D-9E66-4234-9A2C-5D16C6F22B40} = {7C87CAF9-79D7-4C26-9FFB-F3F1FB6911F1}
EndGlobalSection
GlobalSection(ExtensibilityGlobals) = postSolution
SolutionGuid = {55639B5C-0770-4A22-AB56-859604650521}
Expand Down
2 changes: 2 additions & 0 deletions build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<!-- Suppress warnings for repo code using experimental features -->
<NoWarn>$(NoWarn);OTEL1000;OTEL1001</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

We discussed in our meeting a few weeks back anticipating some code taxonomy. A few points of reference for further discussion:

They both take the approach that every 100 or 1000 codes represent a new category of warnings/errors. We could do similar. Another option would be to use a different prefix specifically for feature flags since unlike normal codes, they're ephemeral e.g., OTELFF or something.

I have no strong opinions, just sharing additional thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been thinking about this but so far haven't come up with anything super useful. We could always just say the 1XXX codes are for experimental features and use other codes (eg 2XXX) for some future category?

<!--temporarily disable. See 3958-->
<!--<AnalysisLevel>latest-All</AnalysisLevel>-->
</PropertyGroup>
Expand Down
42 changes: 42 additions & 0 deletions docs/diagnostics/OTEL1000.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# OpenTelemetry .NET Diagnostic: OTEL1000

## Overview

This is an experimental feature diagnostic covering the following APIs:

* `LoggerProviderBuilder`
* `LoggerProvider`
* `IDeferredLoggerProviderBuilder`

Experimental features may be changed or removed in the future.

## Details

The OpenTelemetry Specification defines a `LoggerProvider` as part of its
[API](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md)
&
[SDK](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md)
components.

The SDK allows calling `Shutdown` and `ForceFlush` on the `LoggerProvider` and
also allows processors to be added dynamically to a pipeline after its creation.

Today the OpenTelemetry .NET log pipeline is built on top of the
Microsoft.Extensions.Logging `ILogger` \ `ILoggerProvider` \ `ILoggerFactory`
APIs which do not expose such features.

We also have an issue with the `ILoggingBuilder.AddOpenTelemetry` API in that it
interacts with the `OpenTelemetryLoggerOptions` class. Options classes are NOT
available until the `IServiceProvider` is available and services can no longer
be registered at that point. This prevents the current logging pipeline from
exposing the same dependency injection surface we have for traces and metrics.

We are exposing these APIs to solve these issues and gather feedback about their
usefulness.

## Log Bride API
Copy link
Member

Choose a reason for hiding this comment

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

I'm fairly confident you mean "bridge" not "bride"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @martinjt fixed 👰‍♀️ -> 🌉

Choose a reason for hiding this comment

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

I'm fairly confident you mean "bridge" not "bride"

Way to ruin a perfectly serviceable proposal to the logging API 😏


The OpenTelemetry Specification defines a log bridge API which is rooted off of
the `LoggerProvider` (`GetLogger`) and exposes a `Logger` API to submit log
records. See [OTEL1001](.\OTEL1001.md) for details about the log bridge
implementation status.
34 changes: 34 additions & 0 deletions docs/diagnostics/OTEL1001.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# OpenTelemetry .NET Diagnostic: OTEL1001

## Overview

This is an experimental feature diagnostic covering the following APIs:

* `LoggerProvider.GetLogger`
* `Logger`
* `LogRecordAttributeList`
* `LogRecordData`
* `LogRecordSeverity`

Experimental features may be changed or removed in the future.

## Details

The OpenTelemetry Specification defines a [Logs Bridge
API](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md).

The log bridge API is used by library authors to build log appenders which route
messages from different log frameworks into OpenTelemetry.

Today the OpenTelemetry .NET log pipeline is built on top of the
Microsoft.Extensions.Logging `ILogger` \ `ILoggerProvider` \ `ILoggerFactory`
APIs.

We are exposing these APIs gather feedback about their usefulness. An
alternative approach may be taken which would be to append into `ILogger`
instead of OpenTelemetry directly.

## LoggerProvider API

The OpenTelemetry Specification defines a `LoggerProvider` API. See
[OTEL1000](.\OTEL1001.md) for details about the log implementation status.
1 change: 0 additions & 1 deletion docs/metrics/exemplars/docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,3 @@ services:
- GF_FEATURE_TOGGLES_ENABLE=traceqlEditor
ports:
- "3000:3000"

Copy link
Member

Choose a reason for hiding this comment

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

unintended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya ☹️ I had this file modified locally for a demo I was doing and it got included. I can't seem to get the diff to eliminate it completely.

10 changes: 9 additions & 1 deletion src/OpenTelemetry.Api/Logs/IDeferredLoggerProviderBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@

#nullable enable

#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

#if EXPOSE_EXPERIMENTAL_FEATURES
Expand All @@ -25,6 +30,9 @@ namespace OpenTelemetry.Logs;
/// dependency injection.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LoggerProviderExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand All @@ -34,7 +42,7 @@ namespace OpenTelemetry.Logs;
/// </summary>
internal
#endif
interface IDeferredLoggerProviderBuilder
interface IDeferredLoggerProviderBuilder
{
/// <summary>
/// Register a callback action to configure the <see
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
using System.Collections;
using System.ComponentModel;
using System.Diagnostics;
#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
#endif
using OpenTelemetry.Internal;
using OpenTelemetry.Trace;

Expand All @@ -29,6 +32,9 @@ namespace OpenTelemetry.Logs;
/// Stores attributes to be added to a log message.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand Down
7 changes: 7 additions & 0 deletions src/OpenTelemetry.Api/Logs/LogRecordData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
#nullable enable

using System.Diagnostics;
#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

Expand All @@ -25,6 +29,9 @@ namespace OpenTelemetry.Logs;
/// Stores details about a log message.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand Down
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Api/Logs/LogRecordSeverity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,21 @@

#nullable enable

#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// Describes the severity level of a log record.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand Down
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Api/Logs/LogRecordSeverityExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,21 @@

#nullable enable

#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// Contains extension methods for the <see cref="LogRecordSeverity"/> enum.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand Down
10 changes: 9 additions & 1 deletion src/OpenTelemetry.Api/Logs/Logger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,29 @@

#nullable enable

#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// Logger is the class responsible for creating log records.
/// </summary>
/// <remarks><b>WARNING</b>: This is an experimental API which might change or be removed in the future. Use at your own risk.</remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
/// Logger is the class responsible for creating log records.
/// </summary>
internal
#endif
abstract class Logger
abstract class Logger
{
/// <summary>
/// Initializes a new instance of the <see cref="Logger"/> class.
Expand Down
18 changes: 18 additions & 0 deletions src/OpenTelemetry.Api/Logs/LoggerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif
#if NET8_0_OR_GREATER
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

Expand All @@ -27,6 +30,9 @@ namespace OpenTelemetry.Logs;
/// LoggerProvider is the entry point of the OpenTelemetry API. It provides access to <see cref="Logger"/>.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LoggerProviderExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand All @@ -49,6 +55,9 @@ protected LoggerProvider()
/// Gets a logger.
/// </summary>
/// <returns><see cref="Logger"/> instance.</returns>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public Logger GetLogger()
=> this.GetLogger(name: null, version: null);

Expand All @@ -57,6 +66,9 @@ public Logger GetLogger()
/// </summary>
/// <param name="name">Optional name identifying the instrumentation library.</param>
/// <returns><see cref="Logger"/> instance.</returns>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public Logger GetLogger(string? name)
=> this.GetLogger(name, version: null);

Expand All @@ -66,6 +78,9 @@ public Logger GetLogger(string? name)
/// <param name="name">Optional name identifying the instrumentation library.</param>
/// <param name="version">Optional version of the instrumentation library.</param>
/// <returns><see cref="Logger"/> instance.</returns>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public Logger GetLogger(string? name, string? version)
{
if (!this.TryCreateLogger(name, out var logger))
Expand All @@ -84,6 +99,9 @@ public Logger GetLogger(string? name, string? version)
/// <param name="name">Optional name identifying the instrumentation library.</param>
/// <param name="logger"><see cref="Logger"/>.</param>
/// <returns><see langword="true"/> if the logger was created.</returns>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LogBridgeApiExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
protected virtual bool TryCreateLogger(
string? name,
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
Expand Down
8 changes: 8 additions & 0 deletions src/OpenTelemetry.Api/Logs/LoggerProviderBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,21 @@

#nullable enable

#if NET8_0_OR_GREATER && EXPOSE_EXPERIMENTAL_FEATURES
using System.Diagnostics.CodeAnalysis;
using OpenTelemetry.Internal;
#endif

namespace OpenTelemetry.Logs;

#if EXPOSE_EXPERIMENTAL_FEATURES
/// <summary>
/// LoggerProviderBuilder base class.
/// </summary>
/// <remarks><inheritdoc cref="Logger" path="/remarks"/></remarks>
#if NET8_0_OR_GREATER
[Experimental(DiagnosticDefinitions.LoggerProviderExperimentalFeature, UrlFormat = DiagnosticDefinitions.UrlFormat)]
#endif
public
#else
/// <summary>
Expand Down
1 change: 1 addition & 0 deletions src/OpenTelemetry.Api/OpenTelemetry.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

<ItemGroup>
<Compile Include="$(RepoRoot)\src\Shared\ActivityHelperExtensions.cs" Link="Includes\ActivityHelperExtensions.cs" />
<Compile Include="$(RepoRoot)\src\Shared\DiagnosticDefinitions.cs" Link="Includes\DiagnosticDefinitions.cs" />
<Compile Include="$(RepoRoot)\src\Shared\ExceptionExtensions.cs" Link="Includes\ExceptionExtensions.cs" />
<Compile Include="$(RepoRoot)\src\Shared\Guard.cs" Link="Includes\Guard.cs" />
<Compile Include="$(RepoRoot)\src\Shared\SemanticConventions.cs" Link="Includes\SemanticConventions.cs" />
Expand Down
27 changes: 27 additions & 0 deletions src/Shared/DiagnosticDefinitions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// <copyright file="DiagnosticDefinitions.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

#nullable enable

namespace OpenTelemetry.Internal;

internal static class DiagnosticDefinitions
{
public const string UrlFormat = "https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/diagnostics/{0}.md";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe consider an index page?

https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/experimentat-features/README.md#{0}

This would allow us to delete the individual md files as experimental features are released or perhaps abandoned. The index page could still contain the master list of features over time and the links to could be adjusted as necessary to a prior tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we could ever delete one. Because some shipped code out there will still be pointing to it. All we can do is update say the OTEL1000.md file to note at the top (something like): "This was released as stable in vX.Y.Z and is no longer marked experimental going forward." Or perhaps: "This feature was completely removed in vX.Y.Z and is no longer available going forward."

Should we have individual files or one big one? 🤷 Seems easier to maintain them individually but not sure about that.

Looks like StyleCop has 3 levels. An index showing the categories. An index for each category. And then individual files. When I get a warning from StyleCop in VS (just tested CA1311) it links me directly to the individual file.

Copy link
Member

Choose a reason for hiding this comment

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

I envisioned there would still be individual files, but they could be deleted and links on the index page could be updated to point to a prior permalinked tag/commit. I too noted that codes for other things (stylecop/xunit/csharp), typically link to a dedicated page. Main reason I was considering a different approach is that experimental features are typically short lived. Though no strong opinion here.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK you convinced me this was useful so I implemented it. Take a look at latest and LMK. I also renamed things from "Experimental Feature" to "Experimental API" in the code & docs. I thought that would help differentiate these from the experimental features we typically hide behind envvars.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, looks good! Though bear with me through a little more bikeshedding...

I think the name of the folder diagnostics may get confused with other things. I suggest an organization like:

docs/
└── codeanalysis/
    ├── experimental-apis/
    │   ├── index.md
    │   ├── OTEL1000.md
    │   └── OTEL1001.md
    └── some-future-category/
        └── OTEL2000.md

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest I pushed some updates. To browse it check out: https://github.com/CodeBlanch/opentelemetry-dotnet/tree/api-log-diagnostics/docs/diagnostics

I kept the "diagnostics" folder (didn't rename to "codeanalysis") and I used "README"s instead of "index"s but otherwise I did what you suggested.


public const string LoggerProviderExperimentalFeature = "OTEL1000";
public const string LogBridgeApiExperimentalFeature = "OTEL1001";
}