Conversation
|
Would hugely appreciate if @stbischof / |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7964 +/- ##
=========================================
Coverage 90.12% 90.12%
Complexity 7463 7463
=========================================
Files 834 834
Lines 22585 22585
Branches 2239 2239
=========================================
Hits 20354 20354
Misses 1530 1530
Partials 701 701 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Will Look at this at weekend @jack-berg you May want Thatcher BND also calc your jpms Module info. https://bnd.bndtools.org/chapters/330-jpms.html Maybe. Not on this Step and Pr. But this Couleur be an extra |
Thanks for the tip! My approach is going to be to write a test case showing a scenario that fails, then update the bnd configuration so it passes. So the key will be knowing which scenarios fail without bnd calculating jpms module info, and encoding that into a test. |
@jack-berg I'm guessing you mean me instead of @roytee 😄 , but sure, will try and find some time in the coming week to test everything out :)! What caused you to implement it ;)? |
Been meaning to for a while given how many 👍 the issue has. Was out on parental leave when this comment was left. Long overdue I gave it some attention! |
|
@royteeuwen / @stbischof any chance to take a look at this? 🙂 |
|
Thank you for the reminder! Will Look on this! may its a good first step to post my generates manifest from some months ago. i would recomment to add:
helps to declare all serviceloader-services by adding opentelemetry-context opentelemetry-exporter-otlp In my setup I used this because of split-packages. But currently cant find the reason.
If you plan to have perfect good JMPS modules auto-generated, you should look at this: @timothyjward |
|
My recommendation would be: Try to come to closure here and add basic OSGi metadata, anything else (jpms, metainf-services) could better be added on top of that with additional changes. This might make it also easier to look at diff/differences and one can already make some things work. |
|
Sorry totally missed the notification for the 2/6 comment and thought this was still stale.
I'm new to OSGi so my strategy is to build failing tests to clearly illustrate what fails without a particular feature, then go add that feature to get a passing test. What fails without the Require-Capability / Provide-Capability headers? Seems like SPI mechanisms based on the example? |
|
Ich you use Internal serviceloader mechanism it might be That this Would fail. But i think first step ist to just do export/Import. And we can use That an the help you with further tests. This way we do not overengeneer |
Exactly my thought. Next step would be to write a test case which depends on the SPI mechanism (either autoconfigure, or OTLP exporters loading senders via SPI, etc), watching it fail, and then making it pass (probably by adding If you agree with the code / approach up to this point, consider giving an approval. Approvals are meaningful in this repo even when they don't come from an official approver. Especially so in cases like this outside our core competency. |
|
@jack-berg I finally had the time to go through them and I have some remarks: opentelemetry-exporter-common and opentelemetry-exporter-otlp currently has code to work with either okhttp or with grpc. The way the imports are now defined, it would need both the implementations to be available. So ideally we would set both of them as optional and you can choose which one is the one you want to use (I haven't found a working grpc implementation in OSGi yet, so I went for ok http) To do this, we can set the io.grpc, io.grpc.stub and com.google.common (the packages required for grpc) as resolution:=optional. I'd have to see which ones are the ones for ok http and do the same. for opentelemetry-sdk-trace I'd also set the sun.misc as optional, Unsafe will be removed and is not available everywhere (#6433) opentelemetry-common and opentelemetry-sdk-extension-autoconfigure would need a opentelemetry-sdk-extension-autoconfigure, discovers exporters, samplers, resource providers: opentelemetry-exporter-common, discovers HTTP/gRPC senders: If you want (and agree), I can do these changes on this PR? I could potentially also see if I can add some tests for it :) |
|
Thanks for the review @royteeuwen!
Under the covers they use SPI mechanism to load and resolve the correct sender implementation, gRPC upstream or okhttp or JDK HttpClient. I think we'll need the
👍 all of our usages are resilient to it being unavailable, and have fallback mechanisms.
I was going to open a followup PR for these changes: https://github.com/open-telemetry/opentelemetry-java/pull/7964/changes#diff-2a792e194b8d6465c4f4caea2a7f31fc6179a196def02b0dc07f19d97734ba48R35-R39 My idea is roughly:
I'd rather get something landed and iterate than wait for this PR to have everything. |
I think the general idea is sane. But I can also only again recommend to not make this PR to broad! In general if you require at least one of Regarding the SPI thing: If you are open to get first-class support for OSGi for the sake of some extra code that is not used otherwise you can do the following:
All these annotation are not runtime dependencies so can safely be omitted (that's why the xml is generated) outside the build (provided scope) and you will automatically make things work in OSGi + allow easy extension there. Again, this is something that might be added on top of inital OSGi manifest headers. |
@jack-berg that idea sounds very nice! And thus this would mean that you would have to make both the Import-Package of okhttp and grpc optional, so that if you have a runtime that doesn't have either okhttp or grpc, it would use jdk httpclient, and else can of course also use any of the other two. I can just say that the current PR is not like that, it requires all implementations to be there: opentelemetry-exporter-common.jar/META-INF/MANIFEST.MF: opentelemetry-exporter-otlp.jar/META-INF/MANIFEST.MF: and actually, the opentelemetry-exporter-sender-okhttp even has a dependency on grpc: |
4 participants
Related to #768.
This adds initial OSGi support to all artifacts. Specifically, this uses the
biz.aQute.bnd.builderplugin to enrich each artifact'sMANIFEST.MF. Here's a before and after ofMANIFEST.MFforopentelemetry-api:Before:
After:
The import thing this tool does is specify each artifact's
Export-PackageandImport-Package. Still coming up to speed on OSGi, but my understanding is that this is a specification for the packages each artifact exports for use by others, and the packages each depends on.Ideally, we would exclude our
*.internal.*packages from our exports, but this is blocked by #6970.A key bit we have to do is tell OSGi which packages are optional. For example,
opentelemetry-apihas compileOnly dependencies onopentelemetry-api-incubatorand a reflection interaction withopentelemetry-sdk-extension-autoconfigure. If we don't tell the bnd that these are optional, then OSGi environments fail to initialize when these are missing. I've extended our gradle tooling to make it easy to specify optional packages by settingotelJava.osgiOptionalPackages.Detecting misconfiguration of OSGi around optional packages requires test configurations that exercise a specific set of dependencies. I have an initial test configuration which only includes
opentelemetry-sdkand transitive dependencies, and have done an initial pass of specifying optional dependencies, but more test configurations and more OSGi configuration will be needed.