instrumentation-api-appender questions

[trask]

Tracking some questions that came up during the initial instrumentation-api-appender PR (#4917)

1. maybe instead of calling it "appender api", call it "log emitter api"? (with similarly named package, e.g. io.opentelemetry.instrumentation.api.logemitter)
   • 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"...
2. how to reduce friction for library instrumentation users
   • would be nice for them not to have to add instrumentation-sdk-appender dependency. maybe we can pull it in transitively from the library instrumentation? and even add api surface into the library instrumentation itself to accept the SdkLogEmitterProvider, so user doesn't even need to know about instrumentation-sdk-appender?
3. change package name from io.opentelemetry.instrumentation.api.appender to io.opentelemetry.instrumentation.api.logs.appender, to align with io.opentelemetry.instrumentation.api.log.LoggingContextConstants?
   • should LoggingContextConstants be moved to the appender api?

After writing these down, I think I'm currently in favor of renaming instrumentation-api-appender to instrumentation-api-logs, and moving LoggingContextConstants to that artifact, as it addresses items 1 and 3 above.

[anuraaga]

I think the idea was this is an implementation detail of instrumentated appenders. If there is an end user use case for the API, I think we really should put it in the SDK repo. It looks so similar to the other repos in that API that it's already a smell for it to be separate, if renaming I would probably want to go ahead and move it up.

[mateuszrzeszutek]

A.d. 1&3 👍

and even add api surface into the library instrumentation itself to accept the SdkLogEmitterProvider, so user doesn't even need to know about instrumentation-sdk-appender?

We'd have to somehow force muzzle to ignore these sdk classes - I think we'd need #2556 (or something similar) to make that work.

[trask]

We'd have to somehow force muzzle to ignore these sdk classes - I think we'd need #2556 (or something similar) to make that work.

these methods would be in the library instrumentation, and the javaagent instrumentation wouldn't call them, so I thought muzzle will ignore already?

[trask]

I think the idea was this is an implementation detail of instrumentated appenders. If there is an end user use case for the API, I think we really should put it in the SDK repo. It looks so similar to the other repos in that API that it's already a smell for it to be separate, if renaming I would probably want to go ahead and move it up.

@anuraaga another reason to consider moving it is to improve the U/X for library instrumentation users, as moving it would allow OpenTelemetrySdkBuilder.buildAndRegisterGlobal() to set the global for logs also, instead of those users needing to call GlobalLogEmitterProvider.set() separately.

[trask]

another reason to potentially move it and not wait for a "custom events" user api:

According to Otel spec there is no difference between logs and events. They are totally synonymous words for the exact same concept.

open-telemetry/opentelemetry-specification#2074 (comment)

[anuraaga]

I have filed open-telemetry/opentelemetry-specification#2234 to discuss in the spec more on how to deal with a logs API. We definitely need a separate interface - after writing that, I suspect the most pragmatic will be actually renaming or repackaging to something internal and trying our best not to expose any logs APIs to users until there is an actual logs API in the spec. Though will see how that discussion goes.

Going internal means only using from instrumentation we maintain - as we would have logback, log4j, jul anyways I guess it's practical enough.

[mateuszrzeszutek]

these methods would be in the library instrumentation, and the javaagent instrumentation wouldn't call them, so I thought muzzle will ignore already?

It's enough for them to be present in the helper class to be picked up.

[trask]

we decided to hide the instrumentation api/sdk under internal packages/artifacts, as there is no desire to create anything that could be mistaken for a log api. closing this and will open a new issue to track hiding them (targeted for this week's v1.10.0 release)