Add support for log emitter provider #3994

elevenzqx · 2021-12-15T02:40:39Z

In this CL:

add default noop log emitter, guaranteed to be consistent with tracer and metrics
add logEmitterProvider api
add unit test cases

codecov · 2021-12-15T02:51:07Z

Codecov Report

Merging #3994 (aa30995) into main (cbecf1f) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3994      +/-   ##
============================================
+ Coverage     89.71%   89.96%   +0.25%     
- Complexity     4272     4287      +15     
============================================
  Files           513      515       +2     
  Lines         12941    12965      +24     
  Branches       1248     1250       +2     
============================================
+ Hits          11610    11664      +54     
+ Misses          927      900      -27     
+ Partials        404      401       -3

Impacted Files	Coverage Δ
.../opentelemetry/sdk/logs/SdkLogEmitterProvider.java	`100.00% <ø> (ø)`
...a/io/opentelemetry/sdk/logs/DefaultLogEmitter.java	`100.00% <100.00%> (ø)`
...ntelemetry/sdk/logs/DefaultLogEmitterProvider.java	`100.00% <100.00%> (ø)`
.../io/opentelemetry/sdk/logs/LogEmitterProvider.java	`100.00% <100.00%> (ø)`
...er/otlp/internal/okhttp/OkHttpExporterBuilder.java	`87.03% <0.00%> (-5.83%)`	⬇️
...telemetry/sdk/metrics/SdkMeterProviderBuilder.java	`96.29% <0.00%> (-0.38%)`	⬇️
.../io/opentelemetry/sdk/OpenTelemetrySdkBuilder.java	`100.00% <0.00%> (ø)`
...ry/exporter/otlp/internal/grpc/GrpcStatusUtil.java	`100.00% <0.00%> (ø)`
...xporter/otlp/internal/grpc/ManagedChannelUtil.java	`86.27% <0.00%> (ø)`
...xporter/otlp/internal/grpc/OkHttpGrpcExporter.java	`82.47% <0.00%> (ø)`
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbecf1f...aa30995. Read the comment docs.

anuraaga

@jack-berg Can you check out this PR?

anuraaga · 2021-12-16T05:51:52Z

@trask Do I understand correctly that your PR is the same as this API?

open-telemetry/opentelemetry-java-instrumentation#4917

trask · 2021-12-16T19:35:10Z

@trask Do I understand correctly that your PR is the same as this API?

open-telemetry/opentelemetry-java-instrumentation#4917

I don't think so. open-telemetry/opentelemetry-java-instrumentation#4917 introduces a way for instrumentation to avoid direct dependency on SDK, which we need for javaagent bridging.

jack-berg · 2021-12-17T00:14:07Z

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/GlobalLogEmitterProvider.java

+
+package io.opentelemetry.sdk.logs;
+
+/** This class is a temporary solution until log SDK is marked stable. */


Two thoughts:

Do we want a global accessor for the log sdk? There's not a precedent for providing global accessor for SDK components. GlobalMeterProvider provides access to MeterProvider, not SdkMeterProvider. GlobalOpenTelemetry provides access to OpenTelemetry, not OpenTelemetrySdk. Who would use this?

Related to the javadoc: because there are no plans for a log api, this likely isn't a temporary solution.

Ok, I see and It has been deleted; this can be defined in my business code in a private way

jack-berg · 2021-12-17T00:15:52Z

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/DefaultLogEmitter.java

+  }
+
+  // Noop implementation of LogEmitter.Builder.
+  private static final class NoopSpanBuilder implements LogBuilder {


NoopSpanBuilder => NoopLogBuilder

jack-berg · 2021-12-17T00:16:19Z

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/DefaultLogEmitter.java

+
+  @Override
+  public LogBuilder logBuilder() {
+    return new NoopSpanBuilder();


Can use a singleton here instead of instantiating a new instance each time.

anuraaga · 2021-12-17T04:03:20Z

Hi @elevenzhan - we are currently working on a logs appender API in the instrumentation API here

open-telemetry/opentelemetry-java-instrumentation#4917

This is because we only expect it to be used by log appenders, not by end users. Will that satisfy your use case?

elevenzqx · 2021-12-18T10:11:17Z

Hi @elevenzhan - we are currently working on a logs appender API in the instrumentation API here

open-telemetry/opentelemetry-java-instrumentation#4917

This is because we only expect it to be used by log appenders, not by end users. Will that satisfy your use case?

Hi @anuraaga - I simplified the implementation and removed global provider; javaagent is a greatest way to resolve logs export, but in some unusable scenarios, It cannot overwrite these scenes;

this CR provides a LogEmitterProvider api and noop implementation, that can be used anywhere who want to use log exporter, not just in javaagent

Let's see if this MR is useful?

anuraaga · 2021-12-20T03:10:47Z

that can be used anywhere who want to use log exporter, not just in javaagent

The logs appender API in the java-instrumentation repo is just a library, it will be used for instrumentation both within the javaagent and without. So I think this one is mostly a dupe of that API

elevenzqx · 2021-12-20T12:28:40Z

that can be used anywhere who want to use log exporter, not just in javaagent

The logs appender API in the java-instrumentation repo is just a library, it will be used for instrumentation both within the javaagent and without. So I think this one is mostly a dupe of that API

thanks @anuraaga, I see instrumentation-api-appender is consistent with current MR, so I will close this merge

elevenzqx added 2 commits December 15, 2021 10:20

feat: logs add support for global provider

bbc527a

fix sdk:logs:spotlessJavaCheck

656dd77

elevenzqx requested review from anuraaga, bogdandrutu, carlosalberto, jack-berg, jkwatson and jsuereth as code owners December 15, 2021 02:40

elevenzqx requested a review from a user December 15, 2021 02:40

elevenzqx requested a review from Oberon00 as a code owner December 15, 2021 02:40

anuraaga reviewed Dec 16, 2021

View reviewed changes

jack-berg reviewed Dec 17, 2021

View reviewed changes

fix noop and remove global provider

aa30995

elevenzqx closed this Dec 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for log emitter provider #3994

Add support for log emitter provider #3994

elevenzqx commented Dec 15, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

anuraaga left a comment

anuraaga commented Dec 16, 2021

trask commented Dec 16, 2021

jack-berg Dec 17, 2021

elevenzqx Dec 18, 2021

jack-berg Dec 17, 2021

elevenzqx Dec 18, 2021

jack-berg Dec 17, 2021

elevenzqx Dec 18, 2021

anuraaga commented Dec 17, 2021

elevenzqx commented Dec 18, 2021 •

edited

Loading

anuraaga commented Dec 20, 2021

elevenzqx commented Dec 20, 2021


		package io.opentelemetry.sdk.logs;

		/** This class is a temporary solution until log SDK is marked stable. */

Add support for log emitter provider #3994

Add support for log emitter provider #3994

Conversation

elevenzqx commented Dec 15, 2021 • edited Loading

codecov bot commented Dec 15, 2021 • edited Loading

Codecov Report

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga commented Dec 16, 2021

trask commented Dec 16, 2021

jack-berg Dec 17, 2021

Choose a reason for hiding this comment

elevenzqx Dec 18, 2021

Choose a reason for hiding this comment

jack-berg Dec 17, 2021

Choose a reason for hiding this comment

elevenzqx Dec 18, 2021

Choose a reason for hiding this comment

jack-berg Dec 17, 2021

Choose a reason for hiding this comment

elevenzqx Dec 18, 2021

Choose a reason for hiding this comment

anuraaga commented Dec 17, 2021

elevenzqx commented Dec 18, 2021 • edited Loading

anuraaga commented Dec 20, 2021

elevenzqx commented Dec 20, 2021

elevenzqx commented Dec 15, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

elevenzqx commented Dec 18, 2021 •

edited

Loading