Skip to content

feat: [SDK-4999] emit Kotlin version on remote log resource attrs - #2713

Open
abdulraqeeb33 wants to merge 6 commits into
mainfrom
ar/sdk-4999-kotlin-version-0b84
Open

feat: [SDK-4999] emit Kotlin version on remote log resource attrs#2713
abdulraqeeb33 wants to merge 6 commits into
mainfrom
ar/sdk-4999-kotlin-version-0b84

Conversation

@abdulraqeeb33

@abdulraqeeb33 abdulraqeeb33 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Android host follow-up for OneSignal-KMP-SDK#17 (SDK-4999): supply KotlinVersion.CURRENT so remote logs include ossdk.kotlin_version for dashboard filtering.

Changes

  • Submodule — pins OneSignal-KMP-SDK to KMP One Signal Android SDK getTags can't get all tags from server #17 (267b460, ar/sdk-4999).
  • OtelPlatformProviderkotlinVersion = KotlinVersion.CURRENT.toString().
  • IOtelPlatformProvider / LoggerPlatformProviderAdapter — add abstract kotlinVersion / swiftVersion / additionalVersionAttributes (adapter forwards to the logger path).
  • OtelFieldsTopLevel — mirrors KMP LogFieldsTopLevel (extras first, blank skip, ossdk. prefix normalize) so the legacy otel path stays at parity while both modules coexist.
  • Tests — provider returns KotlinVersion.CURRENT; top-level attrs cover emit / blank omit / extras merge / reserved-key protection.
  • Docs — mark :otel / IOtelPlatformProvider / OtelFactory as SDK-internal (not for public use).

Compatibility note

Adding the new abstract members on IOtelPlatformProvider is a deliberate source-compatibility break for any external implementor of that interface. :otel is published and OtelFactory accepts the type, but this is an SDK-internal injection seam — the only production implementor is OtelPlatformProvider in :core, and there are no known external consumers. We are not adding default methods or a capability interface for this reason.

Merge order

  1. Land OneSignal-KMP-SDK#17 first.
  2. Merge this PR (re-pin submodule to the merged KMP SHA / release tag if the tip moves).

Test plan

  • Code review vs KMP One Signal Android SDK getTags can't get all tags from server #17 contract
  • :OneSignal:core:testDebugUnitTest --tests "*OtelPlatformProviderTest*"
  • :OneSignal:otel:testDebugUnitTest --tests "*OtelFieldsTopLevelTest*"
  • :OneSignal:core:compileDebugKotlin against pinned submodule
  • After KMP merge: re-pin to merged SHA / tagged release via bump workflow when available

Made with Cursor

Pin OneSignal-KMP-SDK to the SDK-4999 commit and wire Android hosts to
supply KotlinVersion.CURRENT so dashboards can filter by language stack.
Mirrors the same attrs on the legacy otel path for parity.

Co-authored-by: abdulraqeeb33 <abdulraqeeb33@users.noreply.github.com>
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • LoggerPlatformProviderAdapter.kt: 0/3 touched executable lines (0.0%) (4 touched lines in diff)
    • 3 uncovered touched lines in this file
  • OtelPlatformProvider.kt: 4/4 touched executable lines (100.0%) (17 touched lines in diff)
  • OtelFieldsTopLevel.kt: 13/13 touched executable lines (100.0%) (39 touched lines in diff)

Overall (aggregate gate)

17/20 touched executable lines covered (85.0% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • LoggerPlatformProviderAdapter.kt: 0.0% (3 uncovered touched lines)

📥 View workflow run

cursoragent and others added 2 commits August 10, 2026 16:59
Emit runtime toolchain extras via additionalVersionAttributes so
dashboards can filter on ART Java language level and device API level
alongside kotlin_version.

Co-authored-by: abdulraqeeb33 <abdulraqeeb33@users.noreply.github.com>
Co-authored-by: abdulraqeeb33 <abdulraqeeb33@users.noreply.github.com>

@fadi-george fadi-george left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues to address before merge:

  • java.specification.version is hardcoded to "0.9" on Android, so ossdk.java_version would be misleading. Robolectric returns the host JDK version and masks this behavior.
  • Default interface getters preserve source compatibility, but older compiled IOtelPlatformProvider implementations may still throw AbstractMethodError.

Also, please trim attribute keys before removing ossdk. to avoid emitting ossdk.ossdk.*.

- Drop ossdk.java_version: ART hardcodes java.specification.version to
  "0.9"; Robolectric hid this by returning the host JDK version.
- Make kotlinVersion/swiftVersion/additionalVersionAttributes required on
  IOtelPlatformProvider so we do not pretend Kotlin defaults give JVM
  binary compatibility without -Xjvm-default=all.
- Trim attribute keys before stripping ossdk. so " ossdk.foo" cannot
  become ossdk.ossdk.foo.

Co-authored-by: abdulraqeeb33 <abdulraqeeb33@users.noreply.github.com>
@fadi-george

Copy link
Copy Markdown
Contributor

One compatibility concern remains: making these properties abstract removes source compatibility but does not prevent older compiled implementations from throwing AbstractMethodError.

The KDoc also says the interface and Android implementation share an artifact, but IOtelPlatformProvider is in :otel while OtelPlatformProvider is in :core. Please use an optional capability interface or explicitly treat this as a breaking API change.

Drop the incorrect same-artifact / AbstractMethodError compatibility framing.
Nobody outside the SDK implements this interface — leave the members abstract.

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

Agreed the KDoc was wrong on the same-artifact claim (IOtelPlatformProvider is in :otel, OtelPlatformProvider in :core).

We're deliberately not adding a capability interface or default methods here though: this isn't a public implementor surface. The only production implementor is our own OtelPlatformProvider, and app/wrapper authors never implement IOtelPlatformProvider. AbstractMethodError for "older compiled external implementors" isn't a real consumer risk for this type.

Updated the KDoc to say SDK-internal only and dropped the misleading binary-compat framing.

@fadi-george

Copy link
Copy Markdown
Contributor

I understand this is intended as SDK-internal, but it is still publicly exposed from the published :otel module, and public OtelFactory methods accept it. KDoc alone does not make it internal.

Adding abstract properties is therefore a source-breaking API change for any external implementor. If we accept that risk because there are no known consumers, please document it as a deliberate compatibility break and update the PR body, which still says defaults preserve compatibility.

@abdulraqeeb33
abdulraqeeb33 marked this pull request as ready for review August 11, 2026 21:19
@abdulraqeeb33
abdulraqeeb33 requested a review from a team as a code owner August 11, 2026 21:19
Document :otel as SDK-internal (not for public use) and call out the
deliberate source-compat break for new abstract IOtelPlatformProvider members.

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

@fadi-george Good catch — agreed. Updated to document :otel / IOtelPlatformProvider / OtelFactory as SDK-internal (not for public implementors), explicitly called out the deliberate source-compat break for the new abstract members, and fixed the PR body so it no longer claims defaults preserve compatibility.

@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

I understand this is intended as SDK-internal, but it is still publicly exposed from the published :otel module, and public OtelFactory methods accept it. KDoc alone does not make it internal.

Adding abstract properties is therefore a source-breaking API change for any external implementor. If we accept that risk because there are no known consumers, please document it as a deliberate compatibility break and update the PR body, which still says defaults preserve compatibility.

Discussed with @fadi-george - the otel module is something that will always be consumed internally. We should deter external users to use it as we won't be supporting external usage of this module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants