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

Break out logs appender API #3880

Closed
wants to merge 1 commit into from

Conversation

jack-berg
Copy link
Member

Resolves #3807.

The most interesting decision is what to name the log appender API artifact / module.

I've put it in :api:logs-appender. I also considered :extensions:logs-appender, but it didn't feel right for :sdk:logs to have a dependency on an extension module. I also chose "logs-appender" instead of "log-appender" for consistency with the plural logs and metrics SDK module names. I'm open to other suggestions.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #3880 (1f6239a) into main (98dec9e) will increase coverage by 0.30%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3880      +/-   ##
============================================
+ Coverage     89.70%   90.00%   +0.30%     
+ Complexity     4230     4204      -26     
============================================
  Files           504      503       -1     
  Lines         13059    12911     -148     
  Branches       1274     1239      -35     
============================================
- Hits          11714    11621      -93     
+ Misses          915      894      -21     
+ Partials        430      396      -34     
Impacted Files Coverage Δ
.../main/java/io/opentelemetry/api/logs/Severity.java 100.00% <ø> (ø)
...etry/exporter/otlp/internal/logs/LogMarshaler.java 69.62% <ø> (ø)
...n/java/io/opentelemetry/sdk/logs/LogProcessor.java 85.71% <ø> (ø)
.../java/io/opentelemetry/sdk/logs/SdkLogBuilder.java 100.00% <ø> (ø)
.../java/io/opentelemetry/sdk/logs/SdkLogEmitter.java 100.00% <ø> (ø)
...o/opentelemetry/sdk/logs/SdkLogEmitterBuilder.java 100.00% <ø> (ø)
.../opentelemetry/sdk/logs/SdkLogEmitterProvider.java 100.00% <ø> (ø)
...lemetry/sdk/logs/SdkLogEmitterProviderBuilder.java 100.00% <ø> (ø)
...io/opentelemetry/sdk/logs/data/LogDataBuilder.java 100.00% <ø> (ø)
...va/io/opentelemetry/sdk/logs/data/LogDataImpl.java 100.00% <ø> (ø)
... and 15 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 98dec9e...1f6239a. Read the comment docs.

@jkwatson
Copy link
Contributor

I don't think we should add an API API for this at this point. There is no spec for it, and I'm not convinced there will be in the near future. Appenders can and should depend directly on the SDK if they need to be sending data to the SDK. Users who need to use an appender can also depend on the SDK, IMO.

@jack-berg
Copy link
Member Author

This appeared to be the preferred direction for a time.

I'm just as happy sticking with only an SDK until something in the spec changes that suggests an API. After all, despite the module name including the -appender suffix, it looks an awful lot like the trace and metric APIs, which is not the current intent on the spec.

I'm fine dropping this, and instead having instrumentation modules place a compileOnly dependency on the log sdk.

@anuraaga
Copy link
Contributor

I think in Java we have a unique situation where the javaagent will add the appender to a user's app (I guess we're planning this). We usually add instrumentation into, such as this custom appender, into the bootstrap classloader affecting the entire app but we very much don't want to add SDKs there, we keep them in the agent classloader.

As long as there's an API split out somewhere, for example opentelemetry-java/sdk/logs-sdk-api (Javalicious!), that would be fine. That being said, if some instrumentation like RUM will want to send "events" via OTel logs, I'm curious how we can get by without an actual logs API in the end. Perhaps both of these points could be raised in the logs group as reasons to reconsider - but if there's no API in the spec, I think for our Java reasons we do need a split here of some sort.

@jkwatson
Copy link
Contributor

I think in Java we have a unique situation where the javaagent will add the appender to a user's app (I guess we're planning this). We usually add instrumentation into, such as this custom appender, into the bootstrap classloader affecting the entire app but we very much don't want to add SDKs there, we keep them in the agent classloader.

As long as there's an API split out somewhere, for example opentelemetry-java/sdk/logs-sdk-api (Javalicious!), that would be fine. That being said, if some instrumentation like RUM will want to send "events" via OTel logs, I'm curious how we can get by without an actual logs API in the end. Perhaps both of these points could be raised in the logs group as reasons to reconsider - but if there's no API in the spec, I think for our Java reasons we do need a split here of some sort.

For RUM, we're planning on designing a RUM API for otel which will be implemented with the various SDKs. The hope is that it'll be RUM specific, though, and not a part of the general use APIs.

I'd be good with an sdk-scoped "appender api" for now until we have more clarity about what (if any) a final logs/appender API might look like.

@jack-berg
Copy link
Member Author

We usually add instrumentation into, such as this custom appender, into the bootstrap classloader affecting the entire app but we very much don't want to add SDKs there, we keep them in the agent classloader.

Sorry if this is a naive question - I haven't studied the java agent's classloader setup: Is there a way to include the appenders in the agent but with a compileOnly dependency on the SDK, such that the app has to have a runtime dependency on the SDK for the appenders to "activate"?

@anuraaga
Copy link
Contributor

@jack-berg - the idea of using a user's library has come to mind, for example to redirect the agent's logs through a user's logback.xml. But it's tricky, there can be dependency conflicts between the agent and the user's app that cause LinkageError or similar problems with the approach. Avoiding dependency conflicts is the motivation for the classpath separation and why we like to keep more complicated components, like the SDK, in the agent classloader to not affect the app.

@jack-berg
Copy link
Member Author

@anuraaga in today's SIG meeting we discussed the possibility of including an appender API in opentelemetry-java-instrumentation instead. The idea being that its not needed outside of the appender use cases so we can avoid possible confusion arising related to what purpose an appender API serves.

@jack-berg
Copy link
Member Author

Closing this in favor of adding an appender API in opentelemetry-java-instrumentation. Created this ticket to track the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log instrumentation should not have to depend on the log SDK
3 participants