refactor: move compose instrumentation to a separate compose package #24

MustafaHaddara · 2025-01-10T17:35:48Z

Which problem is this PR solving?

Move Compose instrumentation from #20 into a separate package.

Short description of the changes

Created a new package io.honeycomb.opentelemetry.android.compose and moved HoneycombInstrumentedComposable and LocalOpenTelemetryRum to that package.
- I considered leaving LocalOpenTelemetryRum in the main package but the compose package needs to read it and I wasn't sure how we felt about making the compose package depend on the main package.
Updated release instructions to cover both packages.
- Right now both packages can have different version numbers, but the build will fail if either package's version number already exists on Maven Central. This feels usable to me but a little gross/suboptimal? It means that a minor or patch change in one package will require a new artifact for the other package, even if there are no changes. I updated our release process to take the version number from the git tag directly. This means that both packages will use the same version number. Based on PR comments, it sounds like we actually want this!

How to verify that this has the expected result

I pushed a v0.0.1-experimental2 tag pointed at the last commit on this branch to test the deployment process. The build ran successfully and pushes both artifacts to maven central. (core package | compose)

I further pushed a v0.0.1-experimental3 tag. Results:

build: https://app.circleci.com/pipelines/github/honeycombio/honeycomb-opentelemetry-android/310/workflows/eecdecfa-d848-40f0-a67c-96072b5ee616
core package: https://central.sonatype.com/artifact/io.honeycomb.android/honeycomb-opentelemetry-android/0.0.1-experimental3
compose package: https://central.sonatype.com/artifact/io.honeycomb.android/honeycomb-opentelemetry-android-compose

MustafaHaddara · 2025-01-14T15:17:42Z

README.md

@@ -172,7 +172,7 @@ class MainActivity : ComponentActivity() {
 ```

 #### Usage
-Wrap your SwiftUI views with `HoneycombInstrumentedComposable(name: String)`, like so:
+Wrap your Composables with `HoneycombInstrumentedComposable(name: String)`, like so:


beekhc

Based on my experiences with the OpenTelemetry SDK, I'd strongly prefer if we have all our libraries use the same version number. Since there will be dependencies between our libraries, whenever customers want to upgrade one of them, they'll have to upgrade all of them, and that turns into a guessing game of what version works with which version. It's a whole lot easier if they can just change the one "honeycomb" version in their libs.versions.toml and get all the right auto-instrumentation versions.

beekhc · 2025-01-14T19:53:15Z

Based on my experiences with the OpenTelemetry SDK, I'd strongly prefer if we have all our libraries use the same version number. Since there will be dependencies between our libraries, whenever customers want to upgrade one of them, they'll have to upgrade all of them, and that turns into a guessing game of what version works with which version. It's a whole lot easier if they can just change the one "honeycomb" version in their libs.versions.toml and get all the right auto-instrumentation versions.

I also think it would be really nice if we can have a release process where we just change the version number in one place, which I assume shouldn't be too hard with gradle. Is this how our Java SDK does it?

https://github.com/honeycombio/honeycomb-opentelemetry-java/blob/46002ce5bd8533c006643eb9324de499227a1c94/build.gradle#L8

MustafaHaddara · 2025-01-14T22:03:12Z

.../src/androidTest/java/io/honeycomb/opentelemetry/android/HoneycombOptionsInstrumentedTest.kt

+        var expectedVersion = System.getenv("CIRCLE_TAG")
+        expectedVersion = expectedVersion?.slice(1 until expectedVersion.length) ?: "0.0.0-DEVELOPMENT"


This (and the similar block in HoneycombOptionsUnitTest below, feels clumsy, but without it, our unit tests would fail only when we do releases (since the app now gets the version number from the CI_TAG variable directly).

Other suggestions/implementations welcome!

I think this particular test is to test that the options are set correctly, and the other one is to test that the resource attributes are set correctly. They aren't tests of our build system.

So, I think both here and there, we should just use BuildConfig.HONEYCOMB_DISTRO_VERSION as the expected value.

If we want a test or verification of our build process setting up the BuildConfig environment variable correctly, I think we should do that elsewhere, explicitly. But I'm not sure if that's necessary at this point. I don't even know what the failure mode would be... that a released SDK doesn't return the version number it was released as? That seems pretty difficult to verify, and would probably need to be a post-build step or something.

MustafaHaddara · 2025-01-15T18:36:48Z

RELEASING.md

 - Push the tag upstream (this will kick off the release pipeline in CI) e.g. `git push origin v1.2.3`
- Copy change log entry for newest version into draft GitHub release created as part of CI publish steps.
-  - Make sure to "generate release notes" in github for full changelog notes and any new contributors
- Publish the github draft release and this will kick off publishing to GitHub and the NPM registry.


and the NPM registry

other oops

I'm not even sure if we need this part about publishing a GitHub release? Is that something we want? It just takes the tag to push to maven.

MustafaHaddara · 2025-01-15T18:37:57Z

RELEASING.md

@@ -1,11 +1,16 @@
 # Releasing

- `Add steps to prepare release`


Based on #25, the only "prepare release" steps were to go through the codebase and manually update the version number.

However, now that we're taking it from the git tag directly, we don't need this at all, and can just skip straight to updating the Changelog + pushing a new tag.

## Which problem is this PR solving? While working on #24, I noticed Android Studio had updated the compiler config in the `.idea` directory to drop down to java compiler 17 (instead of 21). Similarly, I started seeing warnings like ``` > Task :core:compileDebugJavaWithJavac Java compiler version 21 has deprecated support for compiling with source/target version 8. Try one of the following options: 1. [Recommended] Use Java toolchain with a lower language version 2. Set a higher source/target version 3. Use a lower version of the JDK running the build (if you're not using Java toolchain) For more details on how to configure these settings, see https://developer.android.com/build/jdks. To suppress this warning, set android.javaCompile.suppressSourceTargetDeprecationWarning=true in gradle.properties. warning: [options] source value 8 is obsolete and will be removed in a future release warning: [options] target value 8 is obsolete and will be removed in a future release warning: [options] To suppress warnings about obsolete options, use -Xlint:-options. 3 warnings ``` ([job link](https://app.circleci.com/pipelines/github/honeycombio/honeycomb-opentelemetry-android/314/workflows/4554f6fc-9054-484f-b9be-9b4d550befdf/jobs/887)) The documentation for the CircleCI Android orb implies that Java 17 is the default but clearly it's actually using Java 21. ## Short description of the changes This PR updates our CircleCI config to explicitly use 17, and commits the updated Android Studio config to keep local dev environments in line. ## How to verify that this has the expected result [The same job](https://app.circleci.com/pipelines/github/honeycombio/honeycomb-opentelemetry-android/320/workflows/5a1065e9-47d1-4e2f-941e-ac546bec7238/jobs/901) no longer spits out the `Java compiler version 21` warning

beekhc

Overall, this looks good. The only thing I really think should be changed are those tests you asked about.

beekhc · 2025-01-15T20:48:25Z

README.md

@@ -153,7 +153,7 @@ These events may have the following attributes.
 Initialize the `Honeycomb` sdk, and then wrap your entire app in a `CompositionLocalProvider` that provides `LocalOpenTelemetryRum`, as so:

 ```kotlin
-import io.honeycomb.opentelemetry.android.LocalOpenTelemetryRum
+import io.honeycomb.opentelemetry.android.compose.LocalOpenTelemetryRum


I guess we should update the README to tell them they need to add a dependency on the new library?

beekhc · 2025-01-15T21:05:55Z

.../src/androidTest/java/io/honeycomb/opentelemetry/android/HoneycombOptionsInstrumentedTest.kt

+        var expectedVersion = System.getenv("CIRCLE_TAG")
+        expectedVersion = expectedVersion?.slice(1 until expectedVersion.length) ?: "0.0.0-DEVELOPMENT"


I think this particular test is to test that the options are set correctly, and the other one is to test that the resource attributes are set correctly. They aren't tests of our build system.

So, I think both here and there, we should just use BuildConfig.HONEYCOMB_DISTRO_VERSION as the expected value.

If we want a test or verification of our build process setting up the BuildConfig environment variable correctly, I think we should do that elsewhere, explicitly. But I'm not sure if that's necessary at this point. I don't even know what the failure mode would be... that a released SDK doesn't return the version number it was released as? That seems pretty difficult to verify, and would probably need to be a post-build step or something.

beekhc · 2025-01-15T21:07:49Z

compose/src/test/java/io/honeycomb/opentelemetry/android/compose/ExampleUnitTest.kt

+class ExampleUnitTest {
+    @Test
+    fun addition_isCorrect() {
+        assertEquals(4, 2 + 2)


I mean... I dunno... I can see some value leaving a placeholder unit test here, because that's a lot less annoying than adding one later, but this is pretty irrelevant. I'm not sure what to do about it, though.

beekhc · 2025-01-15T21:09:31Z

RELEASING.md

 - Update `CHANGELOG.md` with the changes since the last release.
+  - One quick way to do this is to run:


I'm not entirely sold on this, and I would suggest we not put this part in there right now. I think it works a lot better to add human-readable things to the CHANGELOG as we make commits, and then just change vNext to the real version number when doing a release.

I'm happy to discuss it, and I might be wrong and this way is better, but I'd rather just leave that discussion for later.

oh I see, make changes to CHANGELOG on in each PR, and then just update the heading when we're doing a release. That works for me. I swiped this section from our RELEASING.md in the web distro but I like your pitch better.

beekhc · 2025-01-15T21:10:54Z

RELEASING.md

 - Push the tag upstream (this will kick off the release pipeline in CI) e.g. `git push origin v1.2.3`
- Copy change log entry for newest version into draft GitHub release created as part of CI publish steps.
-  - Make sure to "generate release notes" in github for full changelog notes and any new contributors
- Publish the github draft release and this will kick off publishing to GitHub and the NPM registry.


I'm not even sure if we need this part about publishing a GitHub release? Is that something we want? It just takes the tag to push to maven.

MustafaHaddara changed the title ~~Mh/separate compose package~~ ci: move compose instrumentation to a separate compose package Jan 10, 2025

MustafaHaddara added 2 commits January 14, 2025 10:17

add empty project

987bb2d

make sure to store compose artifacts as well

8667945

MustafaHaddara force-pushed the mh/separate-compose-package branch from f8ab6cb to cdeca26 Compare January 14, 2025 15:17

MustafaHaddara commented Jan 14, 2025

View reviewed changes

MustafaHaddara force-pushed the mh/separate-compose-package branch from cdeca26 to 6c93a35 Compare January 14, 2025 16:01

MustafaHaddara added 2 commits January 14, 2025 11:01

update release documentation

611889a

move HoneycombInstrumentedComposable to new package

20299be

MustafaHaddara force-pushed the mh/separate-compose-package branch from 6c93a35 to 20299be Compare January 14, 2025 16:01

MustafaHaddara added 3 commits January 14, 2025 11:39

update unit test for main package

687cbd1

update version number for compose package

4a4e57f

update version number for main package as well

5599b39

MustafaHaddara marked this pull request as ready for review January 14, 2025 18:58

MustafaHaddara requested a review from a team as a code owner January 14, 2025 18:58

beekhc requested changes Jan 14, 2025

View reviewed changes

MustafaHaddara force-pushed the mh/separate-compose-package branch 2 times, most recently from fb3b22a to ccca974 Compare January 14, 2025 21:00

tie version number to the git tag

7ef4783

MustafaHaddara force-pushed the mh/separate-compose-package branch 2 times, most recently from aec1da5 to 42b0fb3 Compare January 14, 2025 21:59

make sure unit tests look for the correct version number

679f32f

MustafaHaddara force-pushed the mh/separate-compose-package branch from 42b0fb3 to 679f32f Compare January 14, 2025 22:01

MustafaHaddara commented Jan 14, 2025

View reviewed changes

MustafaHaddara requested a review from beekhc January 15, 2025 15:50

update RELEASING.md with new release instructions

1c552d1

MustafaHaddara commented Jan 15, 2025

View reviewed changes

MustafaHaddara changed the title ~~ci: move compose instrumentation to a separate compose package~~ maint, ci: move compose instrumentation to a separate compose package Jan 15, 2025

MustafaHaddara mentioned this pull request Jan 15, 2025

chore: explicitly drop to jdk 17 #30

Merged

MustafaHaddara changed the title ~~maint, ci: move compose instrumentation to a separate compose package~~ refactor: move compose instrumentation to a separate compose package Jan 15, 2025

beekhc requested changes Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move compose instrumentation to a separate compose package #24

refactor: move compose instrumentation to a separate compose package #24

MustafaHaddara commented Jan 10, 2025 •

edited

Loading

MustafaHaddara Jan 14, 2025

beekhc left a comment

beekhc commented Jan 14, 2025

MustafaHaddara Jan 14, 2025 •

edited

Loading

beekhc Jan 15, 2025

MustafaHaddara Jan 15, 2025

beekhc Jan 15, 2025

MustafaHaddara Jan 15, 2025

beekhc left a comment

beekhc Jan 15, 2025

beekhc Jan 15, 2025

beekhc Jan 15, 2025

beekhc Jan 15, 2025

MustafaHaddara Jan 15, 2025

beekhc Jan 15, 2025

		var expectedVersion = System.getenv("CIRCLE_TAG")
		expectedVersion = expectedVersion?.slice(1 until expectedVersion.length) ?: "0.0.0-DEVELOPMENT"

		@@ -1,11 +1,16 @@
		# Releasing

		- `Add steps to prepare release`

		- Update `CHANGELOG.md` with the changes since the last release.
		- One quick way to do this is to run:

refactor: move compose instrumentation to a separate compose package #24

Are you sure you want to change the base?

refactor: move compose instrumentation to a separate compose package #24

Conversation

MustafaHaddara commented Jan 10, 2025 • edited Loading

Which problem is this PR solving?

Short description of the changes

How to verify that this has the expected result

Choose a reason for hiding this comment

beekhc left a comment

Choose a reason for hiding this comment

beekhc commented Jan 14, 2025

MustafaHaddara Jan 14, 2025 • edited Loading

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

beekhc left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaHaddara commented Jan 10, 2025 •

edited

Loading

MustafaHaddara Jan 14, 2025 •

edited

Loading