search

open-telemetry / opentelemetry-java-contrib

https://opentelemetry.io
Apache License 2.0
156 stars 123 forks source link

[inferred spans] Java 8 vs. agrona #1435

Open breedx-splk opened 2 weeks ago

breedx-splk commented 2 weeks ago

Component(s)

No response

Is your feature request related to a problem? Please describe.

This PR tries to update agorna to the latest version: https://github.com/open-telemetry/opentelemetry-java-contrib/pull/1433

It currently breaks the build, because agorna has chosen to require java 17: https://github.com/real-logic/agrona/releases/tag/1.23.0-v2

So now the inferred-spans module cannot update agorna. We should figure out if we want to continue using it and/or require java 17 as well, or something else.

Describe the solution you'd like

Creating this issue mostly for the component owners to figure this out.

Describe alternatives you've considered

No response

Additional context

No response

breedx-splk commented 2 weeks ago

@jackshirazi @SylvainJuge @JonasKunz what do you think?

SylvainJuge commented 2 weeks ago

The usage in inferred spans is mostly for a few collection implementations, so we definitely not require the latest features. Doing an upgrade here means dropping support from Java 8 to 16 which is definitely a big change. We need to discuss this on our side but my gut feeling is that we will probably only keep the 1.22.0 version for now.

JonasKunz commented 2 weeks ago

Yeah let's just stick with 1.22.0. In the elastic-apm-agent we still support Java 7. For that reason agrona wasn't used as a dependency, but the source code of the used collections was copied directly to the repo.

We only use agrona for some primitive-optimized collections (maps and lists). Those don't use any "nasty" features but just plain Java without any java-api calls and therefore are very very very unlikely pose a security risk. I also don't expect that we lose out on significant performance gains as the fundamental algorithms are decades old.

If we ever get a report of a security vulnerability we can move to copying / reimplementing the used primitive collections, but I highly doubt that we'll receive such a vulnerability report.