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] Remove InstrumentationScope class #4436

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 0 additions & 65 deletions src/OpenTelemetry.Api/InstrumentationScope.cs

This file was deleted.

29 changes: 18 additions & 11 deletions src/OpenTelemetry.Api/Logs/Logger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

#nullable enable

using OpenTelemetry.Internal;

namespace OpenTelemetry.Logs;

/// <summary>
Expand All @@ -28,20 +26,23 @@ internal abstract class Logger
/// <summary>
/// Initializes a new instance of the <see cref="Logger"/> class.
/// </summary>
/// <param name="instrumentationScope"><see
/// cref="OpenTelemetry.InstrumentationScope"/>.</param>
protected Logger(InstrumentationScope instrumentationScope)
/// <param name="name">Optional name identifying the instrumentation library.</param>
protected Logger(string? name)
{
Guard.ThrowIfNull(instrumentationScope);

this.InstrumentationScope = instrumentationScope;
this.Name = string.IsNullOrWhiteSpace(name)
? string.Empty
: name!;
}

/// <summary>
/// Gets the <see cref="OpenTelemetry.InstrumentationScope"/> associated
/// with the logger.
/// Gets the name identifying the instrumentation library.
Copy link
Member

Choose a reason for hiding this comment

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

XML comments can be clean up in a later PR, but just noting that...

We can still refer to this as instrumentation scope here. Something like

Suggested change
/// Gets the name identifying the instrumentation library.
/// Gets the instrumentation scope name of the Logger.

or even just dropping the term instrumentation scope, which mimics the docs for Meter

Suggested change
/// Gets the name identifying the instrumentation library.
/// Gets the Logger name.

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 disagree with you that they suck 🤣 but I did keep it ~consistent with what we have in TracerProvider:

/// <param name="name">Name identifying the instrumentation library.</param>
/// <param name="version">Version of the instrumentation library.</param>

I think I'll leave it alone for now and then in a follow-up we can fix it all up?

Copy link
Member

Choose a reason for hiding this comment

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

I think I'll leave it alone for now and then in a follow-up we can fix it all up?

Totally, and you're right we'd probably benefit from an editorial pass over our XML docs in general.

/// </summary>
public string Name { get; }

/// <summary>
/// Gets the version of the instrumentation library.
/// </summary>
public InstrumentationScope InstrumentationScope { get; }
public string? Version { get; private set; }

/// <summary>
/// Emit a log.
Expand All @@ -51,4 +52,10 @@ protected Logger(InstrumentationScope instrumentationScope)
public abstract void EmitLog(
in LogRecordData data,
in LogRecordAttributeList attributes = default);

internal void SetInstrumentationScope(
string? version)
{
this.Version = version;
}
}
54 changes: 39 additions & 15 deletions src/OpenTelemetry.Api/Logs/LoggerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

#nullable enable

#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
using System.Diagnostics.CodeAnalysis;
#endif

namespace OpenTelemetry.Logs;

/// <summary>
Expand All @@ -25,33 +29,53 @@ internal class LoggerProvider : BaseProvider
{
private static readonly NoopLogger NoopLogger = new();

/// <summary>
/// Initializes a new instance of the <see cref="LoggerProvider"/> class.
/// </summary>
protected LoggerProvider()
{
}

/// <summary>
/// Gets a logger.
/// </summary>
/// <returns><see cref="Logger"/> instance.</returns>
public Logger GetLogger()
=> this.GetLogger(new InstrumentationScope());
=> this.GetLogger(name: null, version: null);

/// <summary>
/// Gets a logger with the given name.
/// </summary>
/// <param name="name">Optional name identifying the instrumentation library.</param>
/// <returns><see cref="Logger"/> instance.</returns>
public Logger GetLogger(string name)
=> this.GetLogger(new InstrumentationScope(name));
public Logger GetLogger(string? name)
=> this.GetLogger(name, version: null);

/// <summary>
/// Gets a logger with the given name and version.
/// </summary>
/// <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>
public Logger GetLogger(string? name, string? version)
{
if (!this.TryCreateLogger(name, out var logger))
{
return NoopLogger;
}

logger!.SetInstrumentationScope(version);

return logger;
}

/// <summary>
/// Gets a logger with given instrumentation scope.
/// Try to create a logger with the given name.
/// </summary>
/// <param name="instrumentationScope"><see cref="InstrumentationScope"/>.</param>
/// <returns><see cref="Logger"/>.</returns>
public virtual Logger GetLogger(InstrumentationScope instrumentationScope)
=> NoopLogger;
/// <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>
protected virtual bool TryCreateLogger(
Copy link
Member Author

Choose a reason for hiding this comment

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

This TryCreateLogger may look a little strange, but it accomplishes a number of things...

  • There is only a single virtual method that must be implemented in SDK(s).
  • We can add other GetLogger methods freely that add other things to the instrumentation scope (schema_url, attributes, etc.) maintained on Logger without implementations breaking.
  • There is some stuff in the spec about repeat calls returning the same instances and mutating instances when config changes. We don't support that yet, but in theory separating "Get" from "Create" should allow the base class to implement that stuff if we ever need to.

string? name,
#if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER
Copy link
Member Author

Choose a reason for hiding this comment

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

A note about this condition...

OpenTelemetry.Api currently targets netstandard2.0;net462 so this will never resolve to true for any binary in the package. @TimothyMothra is currently exploring how to best approach this (eg #4293). Once we have a more recent target (or two) this should light up. In the meantime nothing catastrophic will happen just some wonky false-positive warnings about possibly accessing a null variable.

[NotNullWhen(true)]
#endif
out Logger? logger)
{
logger = null;
return false;
}
}
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Api/Logs/NoopLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace OpenTelemetry.Logs;
internal sealed class NoopLogger : Logger
{
public NoopLogger()
: base(instrumentationScope: new())
: base(name: null)
{
}

Expand Down
2 changes: 2 additions & 0 deletions test/OpenTelemetry.Api.Tests/Logs/LogRecordDataTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
// limitations under the License.
// </copyright>

#nullable enable

using System.Diagnostics;
using Xunit;

Expand Down
89 changes: 89 additions & 0 deletions test/OpenTelemetry.Api.Tests/Logs/LoggerProviderTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// <copyright file="LoggerProviderTests.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

using Xunit;

namespace OpenTelemetry.Logs.Tests;

public sealed class LoggerProviderTests
{
[Fact]
public void NoopLoggerReturnedTest()
{
using var provider = new LoggerProvider();

var logger = provider.GetLogger(name: "TestLogger", version: "Version");

Assert.NotNull(logger);
Assert.Equal(typeof(NoopLogger), logger.GetType());

Assert.Equal(string.Empty, logger.Name);
Assert.Null(logger.Version);
}

[Fact]
public void LoggerReturnedWithInstrumentationScopeTest()
{
using var provider = new TestLoggerProvider();

var logger = provider.GetLogger(name: "TestLogger", version: "Version");

Assert.NotNull(logger);
Assert.Equal(typeof(TestLogger), logger.GetType());

Assert.Equal("TestLogger", logger.Name);
Assert.Equal("Version", logger.Version);

logger = provider.GetLogger(name: "TestLogger");

Assert.NotNull(logger);
Assert.Equal(typeof(TestLogger), logger.GetType());

Assert.Equal("TestLogger", logger.Name);
Assert.Null(logger.Version);

logger = provider.GetLogger();

Assert.NotNull(logger);
Assert.Equal(typeof(TestLogger), logger.GetType());

Assert.Equal(string.Empty, logger.Name);
Assert.Null(logger.Version);
}

private sealed class TestLoggerProvider : LoggerProvider
{
protected override bool TryCreateLogger(string? name, out Logger? logger)
{
logger = new TestLogger(name);
return true;
}
}

private sealed class TestLogger : Logger
{
public TestLogger(string? name)
: base(name)
{
}

public override void EmitLog(in LogRecordData data, in LogRecordAttributeList attributes = default)
{
}
}
}