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

Add a logging appender API #4917

Merged
merged 20 commits into from
Dec 18, 2021
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
15 changes: 15 additions & 0 deletions instrumentation-api-appender/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
plugins {
id("otel.java-conventions")
id("otel.animalsniffer-conventions")
id("otel.jacoco-conventions")
id("otel.japicmp-conventions")
id("otel.publish-conventions")
}

group = "io.opentelemetry.instrumentation"

dependencies {
api("io.opentelemetry:opentelemetry-api")

testImplementation("org.assertj:assertj-core")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

import javax.annotation.Nullable;

public final class GlobalLogEmitterProvider {

private static final Object mutex = new Object();

private static volatile LogEmitterProvider globalLogEmitterProvider = null;

@Nullable private static Throwable setGlobalCaller;

/** Returns the registered global {@link LogEmitterProvider}. */
public static LogEmitterProvider get() {
LogEmitterProvider logEmitterProvider = globalLogEmitterProvider;
if (logEmitterProvider == null) {
synchronized (mutex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've stared at this way longer that I should have, and I can't poke any holes in its correctness. 😁

It was initially concerning to me that there were two stateful variables being modified, and only one was officially guarded by the mutex. Having symmetry there would be nice for readability, but I assumed the get() case was trying to avoid the mutex entirely, probably for performance reasons. As we know, the double check that exists here only works correctly when the variable is volatile (which it is here), but we pay a performance penalty for that as well...

So how about changing globalLogEmitterProvider to be an atomic reference, get rid of the mutex, make setGlobalCaller volatile, and use compareAndSet() in the set() implementation and just get() in the get() implementation? Definitely cleaner.

The docs say that the memory semantics of AtomicReference.get() are the same as a volatile variable, but maybe you have deeper insight about the performance not being the same?

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 copied this from GlobalOpenTelemetry, would prefer to stay consistent with whatever approach is blessed in the SDK (and see @anuraaga's recent open-telemetry/opentelemetry-java#4009) 😄

I did take a look at your suggestion, but it seemed like I still needed a coordinating synchronized block in both get and set. I'm likely missing something though.

logEmitterProvider = globalLogEmitterProvider;
if (logEmitterProvider == null) {
set(NoopLogEmitterProvider.INSTANCE);
return NoopLogEmitterProvider.INSTANCE;
}
}
}
return logEmitterProvider;
}

/**
* Sets the {@link LogEmitterProvider} that should be the global instance. Future calls to {@link
* #get()} will return the provided {@link LogEmitterProvider} instance. This should be called
* once as early as possible in your application initialization logic, often in a {@code static}
* block in your main class. It should only be called once - an attempt to call it a second time
* will result in an error. If trying to set the global {@link LogEmitterProvider} multiple times
* in tests, use {@link GlobalLogEmitterProvider#resetForTest()} between them.
*/
public static void set(LogEmitterProvider logEmitterProvider) {
synchronized (mutex) {
if (globalLogEmitterProvider != null) {
throw new IllegalStateException(
"GlobalLogEmitterProvider.set has already been called. GlobalLogEmitterProvider.set "
+ "must be called only once before any calls to GlobalLogEmitterProvider.get. "
+ "Previous invocation set to cause of this exception.",
setGlobalCaller);
}
globalLogEmitterProvider = logEmitterProvider;
setGlobalCaller = new Throwable();
}
}

/**
* Unsets the global {@link LogEmitterProvider}. This is only meant to be used from tests which
* need to reconfigure {@link LogEmitterProvider}.
*/
public static void resetForTest() {
globalLogEmitterProvider = null;
}

private GlobalLogEmitterProvider() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.context.Context;
import java.time.Instant;
import java.util.concurrent.TimeUnit;

/**
* Used to construct and emit logs from a {@link LogEmitter}.
*
* <p>Obtain a {@link LogBuilder} via {@link LogEmitter#logBuilder()}, add properties using the
* setters, and emit the log by calling {@link #emit()}.
*/
public interface LogBuilder {

/** Set the epoch timestamp using the timestamp and unit. */
LogBuilder setEpoch(long timestamp, TimeUnit unit);

/** Set the epoch timestamp using the instant. */
LogBuilder setEpoch(Instant instant);

/** Set the context. */
LogBuilder setContext(Context context);

/** Set the severity. */
LogBuilder setSeverity(Severity severity);

/** Set the severity text. */
LogBuilder setSeverityText(String severityText);

/** Set the name. */
LogBuilder setName(String name);

/** Set the body string. */
LogBuilder setBody(String body);

/** Set the attributes. */
LogBuilder setAttributes(Attributes attributes);

/** Emit the log. */
void emit();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

import javax.annotation.concurrent.ThreadSafe;

/**
* A {@link LogEmitter} is the entry point into a log pipeline.
*
* <p>Obtain a log builder via {@link #logBuilder()}, add properties using the setters, and emit it
* via {@link LogBuilder#emit()}.
*/
@ThreadSafe
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a deep precedent of adding @ThreadSafe to interfaces, but it continues to be weird to me because it's just a hint and it's always possible to implement a very thread unsafe implementation of these interfaces. 🤷🏻

public interface LogEmitter {

/** Return a new {@link LogBuilder} to emit a log. */
LogBuilder logBuilder();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

/** Builder class for creating {@link LogEmitter} instances. */
public interface LogEmitterBuilder {

/**
* Assign an OpenTelemetry schema URL to the resulting {@link LogEmitter}.
*
* @param schemaUrl the URL of the OpenTelemetry schema being used by this instrumentation library
* @return this
*/
LogEmitterBuilder setSchemaUrl(String schemaUrl);

/**
* Assign a version to the instrumentation library that is using the resulting {@link LogEmitter}.
*
* @param instrumentationVersion the version of the instrumentation library
* @return this
*/
LogEmitterBuilder setInstrumentationVersion(String instrumentationVersion);

/**
* Gets or creates a {@link LogEmitter} instance.
*
* @return a log emitter instance configured with the provided options
*/
LogEmitter build();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

public interface LogEmitterProvider {

/**
* Creates a {@link LogEmitterBuilder} instance.
*
* @param instrumentationName the name of the instrumentation library
* @return a log emitter builder instance
*/
LogEmitterBuilder logEmitterBuilder(String instrumentationName);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.context.Context;
import java.time.Instant;
import java.util.concurrent.TimeUnit;

final class NoopLogBuilder implements LogBuilder {

static final LogBuilder INSTANCE = new NoopLogBuilder();

@Override
public LogBuilder setEpoch(long timestamp, TimeUnit unit) {
return this;
}

@Override
public LogBuilder setEpoch(Instant instant) {
return this;
}

@Override
public LogBuilder setContext(Context context) {
return this;
}

@Override
public LogBuilder setSeverity(Severity severity) {
return this;
}

@Override
public LogBuilder setSeverityText(String severityText) {
return this;
}

@Override
public LogBuilder setName(String name) {
return this;
}

@Override
public LogBuilder setBody(String body) {
return this;
}

@Override
public LogBuilder setAttributes(Attributes attributes) {
return this;
}

@Override
public void emit() {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

final class NoopLogEmitter implements LogEmitter {

static final LogEmitter INSTANCE = new NoopLogEmitter();

@Override
public LogBuilder logBuilder() {
return NoopLogBuilder.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

final class NoopLogEmitterBuilder implements LogEmitterBuilder {

static final LogEmitterBuilder INSTANCE = new NoopLogEmitterBuilder();

@Override
public LogEmitterBuilder setSchemaUrl(String schemaUrl) {
return this;
}

@Override
public LogEmitterBuilder setInstrumentationVersion(String instrumentationVersion) {
return this;
}

@Override
public LogEmitter build() {
return NoopLogEmitter.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

final class NoopLogEmitterProvider implements LogEmitterProvider {

static final NoopLogEmitterProvider INSTANCE = new NoopLogEmitterProvider();

@Override
public LogEmitterBuilder logEmitterBuilder(String instrumentationName) {
return NoopLogEmitterBuilder.INSTANCE;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.api.appender;

public enum Severity {
UNDEFINED_SEVERITY_NUMBER(0),
TRACE(1),
TRACE2(2),
TRACE3(3),
TRACE4(4),
DEBUG(5),
DEBUG2(6),
DEBUG3(7),
DEBUG4(8),
INFO(9),
INFO2(10),
INFO3(11),
INFO4(12),
WARN(13),
WARN2(14),
WARN3(15),
WARN4(16),
ERROR(17),
ERROR2(18),
ERROR3(19),
ERROR4(20),
FATAL(21),
FATAL2(22),
FATAL3(23),
FATAL4(24);
Copy link
Contributor

Choose a reason for hiding this comment

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

are these in the sdk (still?). Maybe we could use the numeric constants from the sdk? I dunno.

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, this is copied from sdk. the problem is that we this artifact needs to not depend on the sdk.


private final int severityNumber;

Severity(int severityNumber) {
this.severityNumber = severityNumber;
}

public int getSeverityNumber() {
return severityNumber;
}
}
Loading