Add a logging appender API #4917

trask · 2021-12-16T04:55:44Z

Resolves #4790

Just a shim on top of the Log SDK

LogEmitterProvider
LogEmitterBuilder
LogEmitter
LogBuilder

with an additional no-op implementation, and support for an (optional) global LogEmitterProvider.

trask · 2021-12-16T22:29:13Z

A couple of thoughts:

maybe this should just be part of the instrumentation-api artifact?
- EDIT: and also merge instrumentation-api-annotation-support into instrumentation-api?
maybe instead of calling it "appender api", call it "log emitter api"? (with similarly named package, e.g. io.opentelemetry.instrumentation.api.logemitter)

jack-berg · 2021-12-17T00:32:05Z

This README has references to using the OpenTelemetryAppender. Mind splitting off the section related to library instrumentation and updating to describe the new pattern?

jack-berg

Few comments, but looks good!

...pi/src/main/java/io/opentelemetry/instrumentation/appender/api/GlobalLogEmitterProvider.java

instrumentation-appender-api/build.gradle.kts

...-appender-api/src/main/java/io/opentelemetry/instrumentation/appender/api/SdkLogEmitter.java

...ibrary/src/main/java/io/opentelemetry/instrumentation/log4j/v2_16/OpenTelemetryAppender.java

instrumentation-appender-api/build.gradle.kts

trask · 2021-12-17T03:47:51Z

This README has references to using the OpenTelemetryAppender. Mind splitting off the section related to library instrumentation and updating to describe the new pattern?

👍 I'll add this to #4915 so we can postpone the doc update until 0.10.0 release

trask · 2021-12-17T05:10:03Z

maybe this should just be part of the instrumentation-api artifact?

discussed this with @anuraaga and this appender api is (for the most part) is not really an end-user api (just used for a few appenders), compared to the instrumentation api.

although now thinking about it, the appender api may also be useful as a "event api" for end users to emit events.

which could be a reason to rename "appender api" to "log emitter api"...

(I'd like to gather feedback on these questions, but implement any renames/restructurings as follow-ups, so I can unblock #4927 and start writing some javaagent logging instrumentation in parallel)

...pi/src/main/java/io/opentelemetry/instrumentation/appender/api/GlobalLogEmitterProvider.java

mateuszrzeszutek · 2021-12-17T10:01:39Z

...rc/main/java/io/opentelemetry/instrumentation/appender/api/DelegatingLogEmitterProvider.java

+
+import io.opentelemetry.sdk.logs.SdkLogEmitterProvider;
+
+public final class DelegatingLogEmitterProvider implements LogEmitterProvider {


In the agent these Delegating... classes will have to be loaded by the agent classloader (the api classes will be loaded by bootstrap), right? Does it make sense to at least put them into a separate package? (a separate module would probably be "cleaner", but that may be a bit too much for library instrumentations)

oh no, this is absolutely right!

a separate module would probably be "cleaner", but that may be a bit too much for library instrumentations

I went the separate module route so we don't have to do any weird shadow configuration for now, but I'll add this to the list of things to revisit.

mateuszrzeszutek · 2021-12-17T10:12:47Z

...rc/main/java/io/opentelemetry/instrumentation/appender/api/DelegatingLogEmitterProvider.java

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.appender.api;


WDYT about using ...instrumentation.api.appender? Or even ...instrumentation.api.logs.appender; the appender api would under the same parent package as LoggingContextConstants then

I flipped it, but didn't add the logs level, will add this to list of things to revisit.

breedx-splk

Looks good to me. A couple of comments.

breedx-splk · 2021-12-17T22:19:28Z

...er/src/main/java/io/opentelemetry/instrumentation/api/appender/GlobalLogEmitterProvider.java

+  public static LogEmitterProvider get() {
+    LogEmitterProvider logEmitterProvider = globalLogEmitterProvider;
+    if (logEmitterProvider == null) {
+      synchronized (mutex) {


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?

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.

...ion-api-appender/src/main/java/io/opentelemetry/instrumentation/api/appender/LogEmitter.java

breedx-splk · 2021-12-17T22:24:15Z

...ion-api-appender/src/main/java/io/opentelemetry/instrumentation/api/appender/LogEmitter.java

+ * <p>Obtain a log builder via {@link #logBuilder()}, add properties using the setters, and emit it
+ * via {@link LogBuilder#emit()}.
+ */
+@ThreadSafe


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. 🤷🏻

breedx-splk · 2021-12-17T22:30:36Z

...ation-api-appender/src/main/java/io/opentelemetry/instrumentation/api/appender/Severity.java

+  FATAL(21),
+  FATAL2(22),
+  FATAL3(23),
+  FATAL4(24);


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

ya, this is copied from sdk. the problem is that we this artifact needs to not depend on the sdk.

…strumentation/api/appender/LogEmitter.java Co-authored-by: jason plumb <[email protected]>

trask · 2021-12-17T23:45:15Z

I have opened #4935 to track several follow-ups

* Add logging appender api * noop * Add global * drift * Feedback * compileOnly * fix * fix * Rename instrumentation-appender-api to instrumentation-api-appender * Rename package * Optimization * Split out instrumentation-sdk-appender * Fix * Update instrumentation-api-appender/src/main/java/io/opentelemetry/instrumentation/api/appender/LogEmitter.java Co-authored-by: jason plumb <[email protected]> * Fix * Fix * Fix * Fix sdk-appender package name Co-authored-by: jason plumb <[email protected]>

trask added 4 commits December 15, 2021 20:48

Add logging appender api

bd5feb9

noop

fb3b59a

Add global

9c3711e

drift

76b7245

trask marked this pull request as ready for review December 16, 2021 05:23

trask requested review from anuraaga, iNikem, jkwatson, laurit, mateuszrzeszutek, pavolloffay and tylerbenson as code owners December 16, 2021 05:23

anuraaga mentioned this pull request Dec 16, 2021

Add support for log emitter provider open-telemetry/opentelemetry-java#3994

Closed

trask requested a review from jack-berg December 16, 2021 19:35

jack-berg reviewed Dec 17, 2021

View reviewed changes

trask commented Dec 17, 2021

View reviewed changes

instrumentation-appender-api/build.gradle.kts Outdated Show resolved Hide resolved

trask added 4 commits December 16, 2021 19:06

Feedback

97e36d0

compileOnly

a9f7a91

Merge remote-tracking branch 'upstream/main' into logging-appender-api

03ac605

fix

f0446ea

trask mentioned this pull request Dec 17, 2021

Add logs to testing infra #4927

Merged

fix

00bfbb8

anuraaga approved these changes Dec 17, 2021

View reviewed changes

...pi/src/main/java/io/opentelemetry/instrumentation/appender/api/GlobalLogEmitterProvider.java Outdated Show resolved Hide resolved

mateuszrzeszutek approved these changes Dec 17, 2021

View reviewed changes

mateuszrzeszutek reviewed Dec 17, 2021

View reviewed changes

jack-berg approved these changes Dec 17, 2021

View reviewed changes

trask added 6 commits December 17, 2021 12:42

Merge remote-tracking branch 'upstream/main' into logging-appender-api

863afdb

Rename instrumentation-appender-api to instrumentation-api-appender

e9f668c

Rename package

a4dd3ed

Optimization

89cfa93

Split out instrumentation-sdk-appender

8c56426

Fix

eb45e56

breedx-splk reviewed Dec 17, 2021

View reviewed changes

trask and others added 3 commits December 17, 2021 14:52

Update instrumentation-api-appender/src/main/java/io/opentelemetry/in…

3fc8b23

…strumentation/api/appender/LogEmitter.java Co-authored-by: jason plumb <[email protected]>

Fix

5194d0b

Fix

ac2c744

trask mentioned this pull request Dec 17, 2021

instrumentation-api-appender questions #4935

Closed

Fix

1317914

Fix sdk-appender package name

f22376e

trask merged commit cc9d287 into open-telemetry:main Dec 18, 2021

trask deleted the logging-appender-api branch December 18, 2021 00:09

breedx-splk mentioned this pull request Dec 18, 2021

Simplify the thread-safe handling of the globalLogEmitterProvider #4936

Merged

jack-berg mentioned this pull request Dec 20, 2021

Log instrumentation should not have to depend on the log SDK open-telemetry/opentelemetry-java#3807

Closed

jack-berg mentioned this pull request Jan 10, 2023

Further decouple Events API from Logs/Logger open-telemetry/opentelemetry-specification#3086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a logging appender API #4917

Add a logging appender API #4917

trask commented Dec 16, 2021

trask commented Dec 16, 2021 •

edited

Loading

jack-berg commented Dec 17, 2021

jack-berg left a comment

trask commented Dec 17, 2021

trask commented Dec 17, 2021

mateuszrzeszutek Dec 17, 2021

trask Dec 17, 2021

mateuszrzeszutek Dec 17, 2021

trask Dec 17, 2021

breedx-splk left a comment

breedx-splk Dec 17, 2021

trask Dec 17, 2021

breedx-splk Dec 17, 2021

breedx-splk Dec 17, 2021

trask Dec 17, 2021

trask commented Dec 17, 2021


		import io.opentelemetry.sdk.logs.SdkLogEmitterProvider;

		public final class DelegatingLogEmitterProvider implements LogEmitterProvider {

Add a logging appender API #4917

Add a logging appender API #4917

Conversation

trask commented Dec 16, 2021

trask commented Dec 16, 2021 • edited Loading

jack-berg commented Dec 17, 2021

jack-berg left a comment

Choose a reason for hiding this comment

trask commented Dec 17, 2021

trask commented Dec 17, 2021

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

breedx-splk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask commented Dec 17, 2021

trask commented Dec 16, 2021 •

edited

Loading