Create OtelVersion class at build time. #5365

breedx-splk · 2023-04-10T16:46:28Z

So the current approach for finding the telemetry.sdk.version for the Resource uses getResourceAsStream(). Although this has been working fine, we recently discovered that this can be slow on some platforms.

The approach submitted here generates a java source file that contains the build-time version number, and the source is added to the common project's source tree. This then allows the Resource to just reference the constant, rather than having to read the version dynamically at runtime.

jkwatson · 2023-04-10T16:54:50Z

buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionClassPlugin.kt

+      /**
+       * Autogenerated class do not edit.
+       */
+       public final class OtelVersion {


My only concern with this approach: will devs have to run a gradle build in order to be able to compile in IDEA? Or is IDEA smart enough to run what needs to be run and get this file into the generated source when the project is imported?

Yeah, I'm sure a gradle build step has to happen before Resource can compile. Isn't that the same with autovalue tho?

Another option was to do string substitution on the actual source tree, but that leaves uncommitted changes. Open to alternatives.

Here is one idea of how to compromise: https://gist.github.com/breedx-splk/01278cc3eb94415353cac1f9cdce89c7

The idea is instead of referencing a static field constant, try to do the same with reflection, and if that fails, fall back to the previous behavior. I don't love it.

codecov · 2023-04-10T17:51:12Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08 🎉

Comparison is base (9139bec) 91.20% compared to head (840df35) 91.29%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5365      +/-   ##
============================================
+ Coverage     91.20%   91.29%   +0.08%     
- Complexity     4865     4874       +9     
============================================
  Files           546      549       +3     
  Lines         14369    14352      -17     
  Branches       1351     1352       +1     
============================================
- Hits          13105    13102       -3     
+ Misses          875      863      -12     
+ Partials        389      387       -2

Impacted Files	Coverage Δ
.../java/io/opentelemetry/sdk/resources/Resource.java	`93.47% <100.00%> (+2.91%)`	⬆️

... and 27 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

jack-berg · 2023-04-10T18:00:44Z

we recently discovered that this can be slow on some platforms.

Can you elaborate on this please 🙂

sdk/common/src/main/java/io/opentelemetry/sdk/resources/Resource.java

jack-berg · 2023-04-10T18:03:45Z

sdk/common/build.gradle.kts

  id("otel.animalsniffer-conventions")
 }
+apply<OtelVersionClassPlugin>()


If we do this for :sdk:common, should we do it everywhere?

I only wanted to do this in the one place where it's used. If there are other places that ever need to reference the class, then those modules can apply the plugin. I don't think we need it floating around every module. Feels bloaty.

That seems reasonable. Here are some other places we read the version:

TracerShim

OpenTelemetrySpanBuilderImpl

OtlpUserAgent

And those will still continue work, I didn't remove the properties behavior. Are you suggesting that we should tackle those all in this same PR?

Are you suggesting that we should tackle those all in this same PR?

If we have this pattern, it offers a pretty clean way of accessing the version. Adding a plugin to the gradle build is cleaner than copy / pasting the code to read from the resource.

Doesn't have to be in this PR, but if we like this pattern I think we should standardize on it.

breedx-splk · 2023-04-10T20:11:41Z

we recently discovered that this can be slow on some platforms.

Can you elaborate on this please 🙂

Sure: signalfx/splunk-otel-android#507
A user found it on android after enabling strict mode. That mode detected a file read on the UI thread, which is frowned upon.

jack-berg · 2023-04-10T20:29:10Z

Sure: signalfx/splunk-otel-android#507
A user found it on android after enabling strict mode. That mode detected a file read on the UI thread, which is frowned upon.

The stacktrace indicates that SplunkRum is initialized on a UI thread. I don't know the scope of SplunkRum, but if its anything like OpenTelemetrySdk, it seems like a smell for it to initialize on a UI thread.

breedx-splk · 2023-04-10T20:37:43Z

seems like a smell for it to initialize on a UI thread

Ugh, can we please discuss this aspect of this elsewhere?

jack-berg · 2023-04-10T20:56:25Z

Ugh, can we please discuss this aspect of this elsewhere?

Sure, can you open an issue for this then? Trying to understand if the current getResourceAsStream() approach is slow on some platforms.

breedx-splk · 2023-04-10T21:15:21Z

Sure, can you open an issue for this then? Trying to understand if the current getResourceAsStream() approach is slow on some platforms.

Oh sure #5369

buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionClassPlugin.kt

jack-berg

Couple small comments, but I approve of the direction 👍

…assPlugin.kt Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

breedx-splk added 3 commits April 10, 2023 09:38

fix warning

800c143

create plugin for version class

b274538

wire up version class to Resource

804022f

breedx-splk requested a review from a team April 10, 2023 16:46

jkwatson reviewed Apr 10, 2023

View reviewed changes

breedx-splk added 2 commits April 10, 2023 10:11

checkstyle

4abd4ea

jApiCmp

1905d57

jack-berg reviewed Apr 10, 2023

View reviewed changes

sdk/common/src/main/java/io/opentelemetry/sdk/resources/Resource.java Outdated Show resolved Hide resolved

jack-berg reviewed Apr 10, 2023

View reviewed changes

move to internal package

bb81bb3

jApiCmp

8e01e0d

breedx-splk mentioned this pull request Apr 10, 2023

Tracking getResourceAsStream() performance #5369

Closed

jack-berg reviewed Apr 14, 2023

View reviewed changes

buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionClassPlugin.kt Outdated Show resolved Hide resolved

jack-berg reviewed Apr 14, 2023

View reviewed changes

buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionClassPlugin.kt Outdated Show resolved Hide resolved

jack-berg reviewed Apr 14, 2023

View reviewed changes

buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionClassPlugin.kt Outdated Show resolved Hide resolved

jack-berg reviewed Apr 14, 2023

View reviewed changes

breedx-splk and others added 3 commits April 14, 2023 13:53

Update buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionCl…

3a09de5

…assPlugin.kt Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

Update buildSrc/src/main/kotlin/io/opentelemetry/gradle/OtelVersionCl…

e622d7f

…assPlugin.kt Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>

inline methods

840df35

jkwatson approved these changes Apr 14, 2023

View reviewed changes

jack-berg merged commit 75b83db into open-telemetry:main Apr 15, 2023

breedx-splk mentioned this pull request May 4, 2023

Strict mode violation signalfx/splunk-otel-android#507

Closed

zeitlinger mentioned this pull request May 26, 2023

Issue-152 Add Resource Provider for distribution name and version grafana/grafana-opentelemetry-java#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create OtelVersion class at build time. #5365

Create OtelVersion class at build time. #5365

breedx-splk commented Apr 10, 2023

jkwatson Apr 10, 2023

breedx-splk Apr 10, 2023

breedx-splk Apr 10, 2023

codecov bot commented Apr 10, 2023 •

edited

Loading

jack-berg commented Apr 10, 2023

jack-berg Apr 10, 2023

breedx-splk Apr 10, 2023

jack-berg Apr 10, 2023

breedx-splk Apr 10, 2023

jack-berg Apr 14, 2023

breedx-splk commented Apr 10, 2023

jack-berg commented Apr 10, 2023

breedx-splk commented Apr 10, 2023

jack-berg commented Apr 10, 2023

breedx-splk commented Apr 10, 2023

jack-berg left a comment

Create OtelVersion class at build time. #5365

Create OtelVersion class at build time. #5365

Conversation

breedx-splk commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2023 • edited Loading

Codecov Report

jack-berg commented Apr 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breedx-splk commented Apr 10, 2023

jack-berg commented Apr 10, 2023

breedx-splk commented Apr 10, 2023

jack-berg commented Apr 10, 2023

breedx-splk commented Apr 10, 2023

jack-berg left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 10, 2023 •

edited

Loading