WIP:Add OSGi support to OpenTelemetry project #4627

LWogan · 2022-07-20T18:08:05Z

Use bnd plugin to add OSGi metdata to manifest files to allow them to be used within an OSGi framework

linux-foundation-easycla · 2022-07-20T18:08:08Z

The committers listed above are authorized under a signed CLA.

✅ login: LWogan / name: Lorcan Wogan (72c25f0, 9ffc24a, 64fa0c5, 3283d19, 08c181f)

jkwatson · 2022-07-20T19:13:23Z

@LWogan before we can look at this or accept it, we need you to sign the CLA. Thanks!

codecov · 2022-07-20T19:24:26Z

Codecov Report

Merging #4627 (baf0eb4) into main (d84a111) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head baf0eb4 differs from pull request most recent head a5110ce. Consider uploading reports for the commit a5110ce to get more accurate results

@@             Coverage Diff              @@
##               main    #4627      +/-   ##
============================================
- Coverage     90.04%   90.03%   -0.01%     
+ Complexity     5058     5057       -1     
============================================
  Files           581      581              
  Lines         15591    15591              
  Branches       1491     1491              
============================================
- Hits          14039    14038       -1     
  Misses         1100     1100              
- Partials        452      453       +1

Impacted Files	Coverage Δ
...ava/io/opentelemetry/sdk/internal/RateLimiter.java	`94.11% <0.00%> (-5.89%)`	⬇️

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 d84a111...a5110ce. Read the comment docs.

jkwatson · 2022-07-20T23:33:25Z

I know virtually nothing about OSGi, but this looks ok to me on its surface. @jack-berg do you have any experience with OSGi, or know anyone we can rope in to help?

LWogan · 2022-07-21T08:26:27Z

To add a little explanation of what did does. The bnd instruction will generate OSGi metadata and add it to the manifest files of each jar. OSGi is a framework which relies on this metadata to functionally operate. The OpenTelemetry artifacts cannot be used by anyone running in an OSGi framework without this.

There was some logic already present in the project to set specific manifest attributes, I have preserved these by instructing the bnd operation to add them while adding its automatically generated OSGi metadata.

…DK and Implementation Title/Version to all tasks of type Jar. Only add Automatic module name to tasks named "jar" as it shouldn't matter for other Jar tasks

jack-berg

I'm not familiar with OSGi but I asked around New Relic and a couple peek took a glance and said it looks innocent.

For reference, here's what the :api jar manifest looks like before:

Manifest-Version: 1.0
Automatic-Module-Name: io.opentelemetry.api
Built-By: jberg
Built-JDK: 11.0.15
Implementation-Title: all
Implementation-Version: 1.17.0-SNAPSHOT

and after:

Manifest-Version: 1.0
Automatic-Module-Name: io.opentelemetry.api
Bnd-LastModified: 1658958870198
Built-By: jberg
Built-JDK: 11.0.15
Bundle-ManifestVersion: 2
Bundle-Name: opentelemetry-api
Bundle-SymbolicName: opentelemetry-api
Bundle-Version: 1.17.0.SNAPSHOT
Created-By: 11.0.15 (Eclipse Adoptium)
Implementation-Title: all
Implementation-Version: 1.17.0-SNAPSHOT
Import-Package: io.opentelemetry.context,io.opentelemetry.context.prop
 agation,io.opentelemetry.sdk.autoconfigure,javax.annotation;version="
 [3.0,4)"
Private-Package: io.opentelemetry.api,io.opentelemetry.api.baggage,io.
 opentelemetry.api.baggage.propagation,io.opentelemetry.api.common,io.
 opentelemetry.api.internal,io.opentelemetry.api.metrics,io.openteleme
 try.api.trace,io.opentelemetry.api.trace.propagation,io.opentelemetry
 .api.trace.propagation.internal
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.8))"
Tool: Bnd-6.3.1.202206071316

jack-berg · 2022-07-27T21:53:52Z

buildSrc/build.gradle.kts

  // When updating, update above in plugins too
+  implementation("biz.aQute.bnd:biz.aQute.bnd.gradle:6.3.1")
  implementation("com.diffplug.spotless:spotless-plugin-gradle:6.4.2")


The comment applies to the spotless plugin. Should change the order to restore it.

Suggested change

// When updating, update above in plugins too

implementation("biz.aQute.bnd:biz.aQute.bnd.gradle:6.3.1")

implementation("com.diffplug.spotless:spotless-plugin-gradle:6.4.2")

// When updating, update above in plugins too

implementation("com.diffplug.spotless:spotless-plugin-gradle:6.4.2")

implementation("biz.aQute.bnd:biz.aQute.bnd.gradle:6.3.1")

jack-berg · 2022-07-27T22:00:56Z

buildSrc/src/main/kotlin/otel.java-conventions.gradle.kts

@@ -126,6 +124,15 @@ tasks {
    }
  }

+  named("jar", Jar::class.java) {


Can you add a comment indicating that this adds required OSGi metadata to the jar manifest?

…portable from other modules. Various modules within this repo have packages with the namespace internal which are exposed via the apis of non internal classes and therefore had to be exposed

LWogan · 2022-07-28T14:17:33Z

@jack-berg Ive made some additional changes. I needed to add annotations to numerous package-info files. This informs the OSGi framework that these packages are available for use by other jars. Worth noting some packages marked with the namespace internal have been included as some of their classes are on the APIs of classes in other packages.

Another point to add is this PR will add support to this project for OSGi. However there are numerous dependencies such as io.grpc (and its transitive dependencies) which also do not have OSGi support. There are workarounds to this but I thought I should point that out.

Leaving as a draft for now as I am still integrating OpenTelemetry into an OSGi project so I might not have worked out all the issues yet.

splatch · 2022-08-08T23:52:52Z

I was able to test it and I believe it needs a second touch. Import of javax.annotation package should be marked as optional. Otherwise runtime environment is forced to host findbugs which is strictly speaking build time dependency/tool which does not serve any purpose for execution of byte code.

splatch · 2022-08-09T19:59:44Z

@jack-berg Looking at this again (on top of 0.16) I think there are more things to tackle within library design itself.

The SpanExporterConfiguration introduces a lookup of a specific implementations from a dependency consumed in opentelemetry-api module:

Its not quite clear to me why such design exists. With such way in place you tie autconfigure module with all listed spi providers. Effectively it is not possible to separate api from specific implementations as bndtools which generates OSGi metadata is able to spot somehow Class.forName and refered class names. Effectively it causes direct listing of such packages as import statements for for sdk-extensions-autoconfigure, which is used by api. Wouldn't it be better to have some kind of ExporterFactory looked up through ServiceLoader first and then let it configure specific exporter through passed properties?

Any use of Class.forName(String) without a ClassLoader is a trouble marker under OSGi cause it will fail in most of the cases:

Such patterns work fine with single classloader which is used with flat classpath, but will certainly fail with hierarchical classloaders and osgi which has multiple classloaders. For simplicity of understanding - under OSGi each bundle (deployed jar) have its own classloader exposing only declared import statements.

github-actions · 2022-08-23T20:31:04Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jack-berg · 2022-08-23T21:12:35Z

Its not quite clear to me why such design exists.

Hi @splatch - sorry for the delayed reply. Not sure what the best answer is here, but I can provide some context. The autoconfigure is responsible for configuring an OpenTelemetrySdk instance based on system properties and env vars. The components it configures includes exporters for specific protocols, such as zipkin, jaeger, prometheus, otlp, etc. Autoconfigure doesn't want to impose transitive dependencies on those exporters, since users will likely only need one or two and the others would just result in unnecessary bulk. So instead it places compileOnly dependencies on these components, and executes the configuration logic for each conditionally based on its presence on the classpath.

The advantage of this is that configuration lives in one place, instead of being scattered around each of the components. Not sure how you could accomplish this with the ExporterFactory pattern you describe, but admittedly I may not be fully grasping your suggestion.

github-actions · 2022-09-06T21:30:14Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-09-20T21:31:45Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

PascalDutch · 2022-10-25T10:06:55Z

Hi, why is this never merged? We are also looking for functionality like this, since we want to use OpenTelemetry in an OSGi context.

LWogan · 2022-10-25T10:53:44Z

Hi, why is this never merged? We are also looking for functionality like this, since we want to use OpenTelemetry in an OSGi context.

From my perspective the pr is incomplete. Needs more testing and probably more changes.

The package structures of this Otel project means if you want to pull in a single lib then you pretty much have to pull in every other subproject due to the internal dependency tree with each subproject having some package import from another. This is a frustration when you only wish to use a subset of bundles.

Also, unfortunately, many of OpenTelems dependencies and also transient dependencies are not OSGi bundles which makes it very difficult to use this library within an OSGi context even with this PR applied.

LWogan added 4 commits July 20, 2022 17:02

Add OSGi metadata to the API and SDK packages

72c25f0

Add OSGi metadata to the API and SDK packages

9ffc24a

Fix typo in formatting

64fa0c5

Remove need for a bnd file to hide errors from multiple java dirs

3283d19

LWogan requested review from jack-berg, jkwatson and jsuereth as code owners July 20, 2022 18:08

LWogan requested a review from a user July 20, 2022 18:08

LWogan requested a review from Oberon00 as a code owner July 20, 2022 18:08

LWogan mentioned this pull request Jul 20, 2022

Add OSGi support for API and SDK #768

Open

typo

08c181f

LWogan added 4 commits July 21, 2022 11:51

Minimize changes to the original code. Preserve attributes Built-By/J…

ef648b3

…DK and Implementation Title/Version to all tasks of type Jar. Only add Automatic module name to tasks named "jar" as it shouldn't matter for other Jar tasks

move inputs property call into the instruction that needs it

a799771

Simplify syntax to extract the two bnd instructions to two calls

e2b1fe8

Ensure otelJava.moduleName is always evaluated lazily

baf0eb4

LWogan changed the title ~~Add OSGi support to OpenTelemetry project~~ WIP:Add OSGi support to OpenTelemetry project Jul 21, 2022

LWogan marked this pull request as draft July 21, 2022 14:00

jack-berg approved these changes Jul 27, 2022

View reviewed changes

Add @export to package-info.java files for packages that should be im…

a5110ce

…portable from other modules. Various modules within this repo have packages with the namespace internal which are exposed via the apis of non internal classes and therefore had to be exposed

jack-berg self-requested a review July 28, 2022 15:20

github-actions bot added the Stale label Aug 23, 2022

github-actions bot removed the Stale label Aug 23, 2022

github-actions bot added the Stale label Sep 6, 2022

github-actions bot closed this Sep 20, 2022

deejgregor mentioned this pull request Dec 7, 2022

GlobalOpenTelemetry trigger of autoconfiguration is opt-in #5010

Merged

jack-berg mentioned this pull request Aug 26, 2024

solves #768: Add OSGi support by exporting all java packages except internal packages #6669

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP:Add OSGi support to OpenTelemetry project #4627

WIP:Add OSGi support to OpenTelemetry project #4627

LWogan commented Jul 20, 2022

linux-foundation-easycla bot commented Jul 20, 2022 •

edited

Loading

jkwatson commented Jul 20, 2022

codecov bot commented Jul 20, 2022 •

edited

Loading

jkwatson commented Jul 20, 2022

LWogan commented Jul 21, 2022

jack-berg left a comment

jack-berg Jul 27, 2022

jack-berg Jul 27, 2022

LWogan commented Jul 28, 2022

splatch commented Aug 8, 2022

splatch commented Aug 9, 2022

github-actions bot commented Aug 23, 2022

jack-berg commented Aug 23, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 20, 2022

PascalDutch commented Oct 25, 2022

LWogan commented Oct 25, 2022

WIP:Add OSGi support to OpenTelemetry project #4627

WIP:Add OSGi support to OpenTelemetry project #4627

Conversation

LWogan commented Jul 20, 2022

linux-foundation-easycla bot commented Jul 20, 2022 • edited Loading

jkwatson commented Jul 20, 2022

codecov bot commented Jul 20, 2022 • edited Loading

Codecov Report

jkwatson commented Jul 20, 2022

LWogan commented Jul 21, 2022

jack-berg left a comment

Choose a reason for hiding this comment

jack-berg Jul 27, 2022

Choose a reason for hiding this comment

jack-berg Jul 27, 2022

Choose a reason for hiding this comment

LWogan commented Jul 28, 2022

splatch commented Aug 8, 2022

splatch commented Aug 9, 2022

github-actions bot commented Aug 23, 2022

jack-berg commented Aug 23, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 20, 2022

PascalDutch commented Oct 25, 2022

LWogan commented Oct 25, 2022

linux-foundation-easycla bot commented Jul 20, 2022 •

edited

Loading

codecov bot commented Jul 20, 2022 •

edited

Loading