Support App diagnostics endpoint features #76

KiKoS0 · 2024-09-07T00:53:56Z

Summary

Implements the introspection specification, the closest implementation should be the JS SDK: https://github.com/inngest/inngest-js/blob/56ed5c11081517db2a72ae27c83cbf4263d9b6ed/packages/inngest/src/components/InngestCommHandler.ts#L1101-L1179
I left out 2 fields total from the authenticated introspection payload as I believe they are optional and not yet implemented in the Kotlin SDK:

capabilities: this one is actually missing from the spec, I'm just assuming it's optional so let me know if I'm wrong and it needs to be an empty object instead.
extra: it's not used in the Python SDK.

I introduced 2 test dependencies for mocking environment variables in tests and creating inngest clients in production mode.
It's not ideal but it's a real pain, we can look it into better ways to create different test clients in the future.
The mocking approaches are from:

Changes

Add introspection payload classes.
Repurpose the GET /api/inngest to return the introspect payload in both adapters.
Add testing libraries to mock environment variables.

Checklist

Update documentation
Added unit/integration tests

I don't like this at all but it works and it's only a test dependency. https://github.com/webcompere/system-stubs?tab=readme-ov-file#system-stubs

⚠ WARNING: JDK Support.
This project has now moved to a JDK11 minimum version

The v2.x branch is the LTS version. However, there is best effort support to keep the Java 8 compatible
v1.x branch.

I see, that sucks we can't just use one version for everything but this seems like an OK solution given they've moved their LTS to a newer JDK than inngest-kt's minimum

KiKoS0 · 2024-09-07T23:57:44Z

inngest/src/main/kotlin/com/inngest/Comm.kt

+                            apiOrigin = "${config.baseUrl()}/",
+                            framework = framework.value,
+                            sdkVersion = Version.getVersion(),
+                            sdkLanguage = "java",


I went with java as the sdk language following this: https://linear.app/inngest/project/javakotlin-sdk-0e841a80efe2#projectUpdate-dd0137ce&comment-1288aae4
Let me know if we want a different value.

albertchae

great work. mostly minor comments except the one about getting the request body in the spring boot adapter's controller unless I missed something there.

albertchae · 2024-09-11T08:42:54Z

inngest/src/main/kotlin/com/inngest/Environment.kt

    }

+    fun isInngestEventKeySet(value: String?): Boolean =
+        when {
+            value.isNullOrEmpty() -> false


is this because the empty case isn't being handled properly by the elvis operator on line 18? nulls would get replaced by the DUMMY_KEY_EVENT right?

exactly yeah i should probably check if other SDKs are using a null value or not.

Python sdk sets the same dummy value as well: https://github.com/inngest/inngest-py/blob/2bc0fb51d9b05a15c19a0a92c61602823beded49/inngest/_internal/client_lib/client.py#L29 but it doesn't seem to populate the the self._event_key property with it: https://github.com/inngest/inngest-py/blob/2bc0fb51d9b05a15c19a0a92c61602823beded49/inngest/_internal/client_lib/client.py#L99

Instead it conditionally uses it when self._event_key is None: https://github.com/inngest/inngest-py/blob/2bc0fb51d9b05a15c19a0a92c61602823beded49/inngest/_internal/client_lib/client.py#L137-L141

inngest/src/main/kotlin/com/inngest/ServeConfig.kt

inngest/src/test/kotlin/com/inngest/ServeConfigTest.kt

albertchae · 2024-09-11T09:26:17Z

inngest/src/main/kotlin/com/inngest/Introspection.kt

+    open val hasSigningKey: Boolean,
+    open val mode: String,
+    @Json("authentication_succeeded") open val authenticationSucceeded: Boolean?,
+    @Json("schema_version") val schemaVersion: String = "2024-05-24",


is it possible to use @JsonNaming(PropertyNamingStrategy.SnakeCaseStrategy.class) so you don't have to manually snake case every property? I found this on https://stackoverflow.com/questions/10519265/jackson-overcoming-underscores-in-favor-of-camel-case

great suggestion i was really hoping for it to work but it doesn't seem that it works with Klaxon and i don't think it has an equivalent for it either. I wonder if a custom converter would work though.

oh right I missed the SO thread was about Jackson specifically. This could be nice as a follow up but non blocking for this PR

inngest/src/main/kotlin/com/inngest/Introspection.kt

albertchae · 2024-09-11T09:35:05Z

inngest-spring-boot-adapter/src/main/java/com/inngest/springboot/InngestController.java

-        }
-        String response = commHandler.introspect(origin);
-        return ResponseEntity.ok().headers(getHeaders()).body(response);
+        String requestBody = "";


don't we need to get the actual request body somehow?

hmm i believe it gave me an error trying to get the body of a GET request and later on trying it in cloud mode, it didn't seem to be sending a body. I'll try again to get it.

Oh I forgot this is a GET, so yeah you're right there probably won't be a request body. Should we make the argument optional then?

Also I made sure that the JS sdk returns an empty string for GET as well:
https://github.com/inngest/inngest-js/blob/56ed5c11081517db2a72ae27c83cbf4263d9b6ed/packages/inngest/src/components/InngestCommHandler.ts#L729-L735

albertchae · 2024-09-11T09:37:47Z

inngest-spring-boot-demo/build.gradle.kts

+    if (JavaVersion.current().isJava11Compatible) {
+        testImplementation("uk.org.webcompere:system-stubs-jupiter:2.1.6")
+    } else {
+        testImplementation("uk.org.webcompere:system-stubs-jupiter:1.2.1")
+    }


I see, that sucks we can't just use one version for everything but this seems like an OK solution given they've moved their LTS to a newer JDK than inngest-kt's minimum

inngest-spring-boot-demo/build.gradle.kts

albertchae · 2024-09-11T09:40:05Z

...st-spring-boot-demo/src/test/java/com/inngest/springbootdemo/CloudModeIntrospectionTest.java

+@ExtendWith(SystemStubsExtension.class)
+public class CloudModeIntrospectionTest {
+
+    private static final String productionSigningKey = "signkey-prod-2a89e554826a40672684e75eee6e34909b45aa4fd04fff5ff49bbe28c24ef424";


just curious where did you get this key? I mostly stuck to the one in https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#414-requirements-when-sending-a-request in other tests using a prod like signing key IIRC

i generated a signing key and event key on the platform and deleted them. In the end I didn't even use the event key because i wanted to use the same value/hash combination that the Python sdk is using in: https://github.com/inngest/inngest-py/blob/6dcae0b4726f8edaa3d45015fdca9b21be7486b6/tests/base.py#L199
I'll remove this one as well.

Ok cool, using the same as Python sdk also sounds good to me

...st-spring-boot-demo/src/test/java/com/inngest/springbootdemo/CloudModeIntrospectionTest.java

I also added junit-pioneer as a test dependency to be able to set environment variables in certain tests. I followed this guide for now but we can look into mocking the environment ourselves later on: https://www.baeldung.com/java-unit-testing-environment-variables#setting-environment-variables-with-junit-pioneer

I also left a few TODOs for unsupported fields: - capabilities - signing key fallback The `extra` field was also left out, the spec says it's optional and I think only the JS SDK uses it: https://github.com/inngest/inngest/blob/main/docs/SDK_SPEC.md#45-introspection-requests

It's a pain to mock the environment variables in the tests. I introduced a different `system-stubs-jupiter` library to help with that because `org.junitpioneer.jupiter` did not work as expected in a Spring Boot environment. Ideally I think we should have a mockable custom `Environment` interface that we use throughout the application, instead of reaching for `System.getenv()` directly. The method for mocking that worked is described in this guide: https://www.baeldung.com/java-system-stubs#environment-and-property-overrides-for-junit-5-spring-tests

albertchae

LGTM, thanks for corroborating questions with other SDKs

KiKoS0 requested a review from albertchae September 7, 2024 00:53

KiKoS0 self-assigned this Sep 7, 2024

KiKoS0 force-pushed the riadh/inn-3337-support-introspect branch 6 times, most recently from 4944756 to a715755 Compare September 7, 2024 22:24

KiKoS0 commented Sep 7, 2024

View reviewed changes

KiKoS0 force-pushed the riadh/inn-3337-support-introspect branch from a715755 to 925105c Compare September 7, 2024 23:03

KiKoS0 marked this pull request as ready for review September 7, 2024 23:24

KiKoS0 requested review from djfarrelly, darwin67, jpwilliams and tonyhb as code owners September 7, 2024 23:24

KiKoS0 force-pushed the riadh/inn-3337-support-introspect branch from 925105c to 5aaa32a Compare September 7, 2024 23:53

KiKoS0 commented Sep 7, 2024

View reviewed changes

KiKoS0 mentioned this pull request Sep 8, 2024

Add SDK version in format java:v{version} to the sync call #77

Merged

2 tasks

albertchae reviewed Sep 11, 2024

View reviewed changes

KiKoS0 added 8 commits September 12, 2024 02:03

Implement isInngestEventKeySet helper function

6465be2

Repurpose introspect to return the correct introspection payload

feec655

Adapt both adapters according to the new introspect signature

3d1dd29

Use the same signing key that was used by other tests

ad202cc

Change order of introspection fields to alphabetical order

c2804ef

KiKoS0 force-pushed the riadh/inn-3337-support-introspect branch 2 times, most recently from 4f9b4b9 to 1822df5 Compare September 13, 2024 00:45

Address PR feedback

7da679e

KiKoS0 force-pushed the riadh/inn-3337-support-introspect branch from 1822df5 to 7da679e Compare September 13, 2024 00:54

albertchae approved these changes Sep 13, 2024

View reviewed changes

KiKoS0 merged commit e2e6151 into main Sep 13, 2024
9 checks passed

KiKoS0 deleted the riadh/inn-3337-support-introspect branch September 13, 2024 01:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support App diagnostics endpoint features #76

Support App diagnostics endpoint features #76

KiKoS0 commented Sep 7, 2024 •

edited

Loading

linear bot commented Sep 7, 2024

KiKoS0 Sep 7, 2024 •

edited

Loading

albertchae Sep 11, 2024

KiKoS0 Sep 7, 2024

albertchae left a comment

albertchae Sep 11, 2024

KiKoS0 Sep 12, 2024

KiKoS0 Sep 13, 2024

albertchae Sep 11, 2024

KiKoS0 Sep 12, 2024

albertchae Sep 12, 2024

albertchae Sep 11, 2024

KiKoS0 Sep 12, 2024

albertchae Sep 12, 2024

KiKoS0 Sep 13, 2024

albertchae Sep 11, 2024

albertchae Sep 11, 2024

KiKoS0 Sep 12, 2024

albertchae Sep 12, 2024

albertchae left a comment

Support App diagnostics endpoint features #76

Support App diagnostics endpoint features #76

Conversation

KiKoS0 commented Sep 7, 2024 • edited Loading

Summary

Changes

Checklist

Related

linear bot commented Sep 7, 2024

KiKoS0 Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertchae 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

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

Choose a reason for hiding this comment

albertchae left a comment

Choose a reason for hiding this comment

KiKoS0 commented Sep 7, 2024 •

edited

Loading

KiKoS0 Sep 7, 2024 •

edited

Loading