fix: overhaul endpoint discovery #1221

ianbotsf · 2025-01-15T22:35:04Z

Issue #

Description of changes

This change overhauls endpoint discovery to address several missed features and a violation of the specification. In summary:

Refactor generated <Service>EndpointDiscoverer classes into interfaces so that custom implementations can be provided
- Default implementations are now codegenned as Default<Service>EndpointDiscoverer
Refactor the ReadThroughCache class into an ExpiringKeyedCache interface so that custom implementations can be provided
- The current implementation of ReadThroughCache is now provided in PeriodicSweepCache, which implements the new interface
Favor endpointUrl over endpoint discovery when both are provided/enabled. This allows callers to bypass endpoint discovery when using a custom endpoint.
In the codegen layer, provide CodegenContext as a context value in KotlinWriter, which cleans up several duplicated context keys and multiple places where context was unnecessarily handled/duplicated
Fix a bug that prevented DiscoveredEndpointErrorInterceptor from being added to relevant operations

⚠️ Breaking change: This is a breaking change because of the type/name changes for endpoint discoverers and caches.

ℹ️ Companion PR: (link coming soon)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…nd to make endpoint discoverers interfaces so custom implementations can be provided

…discovery

lauzadis · 2025-01-17T14:56:45Z

...on/smithy/kotlin/codegen/rendering/endpoints/discovery/DefaultEndpointDiscovererGenerator.kt

+        delegator.applyFileWriter(symbol) {
+            dokka(
+                """
+                    A class which looks up specific endpoints for ${ctx.settings.sdkId} calls via the `$operationName`


Should ${ctx.settings.sdkId} be the sanitized ${clientName(ctx.settings.sdkId)}?

Yes, good point.

lauzadis · 2025-01-17T14:59:08Z

...on/smithy/kotlin/codegen/rendering/endpoints/discovery/DefaultEndpointDiscovererGenerator.kt

+                RuntimeTypes.Core.Net.Host,
+                RuntimeTypes.Core.Collections.PeriodicSweepCache,
+                KotlinTypes.Time.minutes,
+                RuntimeTypes.Core.Clock,


Clock.System seems to be the default value here, so do we have to set it?

No, we don't.

lauzadis · 2025-01-17T15:10:28Z

...e/amazon/smithy/kotlin/codegen/rendering/endpoints/discovery/EndpointDiscoveryIntegration.kt

-        val endpointDiscoveryOptional = ctx
+    companion object {
+        const val CLIENT_CONFIG_NAME = "endpointDiscoverer"
+        const val ORDER: Byte = 0 // doesn't depend on any other integrations


The default value is zero, so can this be removed?

I now see this is used in aws-sdk-kotlin: https://github.com/awslabs/aws-sdk-kotlin/pull/1506/files#diff-707de3299374cf5e83fa4c28d54258c5e4f90cd575efa9da15580d454657d953R25

👍 Yes, I wanted to clearly document the relationship between the two integrations.

lauzadis · 2025-01-17T15:14:13Z

...e/amazon/smithy/kotlin/codegen/rendering/endpoints/discovery/EndpointDiscoveryIntegration.kt

        ctx: ProtocolGenerator.GenerationContext,
        resolved: List<ProtocolMiddleware>,
-    ): List<ProtocolMiddleware> = super.customizeMiddleware(ctx, resolved) + listOf(DiscoveredEndpointMiddleware)
+    ): List<ProtocolMiddleware> = super.customizeMiddleware(ctx, resolved) + listOf(DiscoveredEndpointErrorMiddleware)


simplification: All other customizeMiddleware implementations just do resolved + listOf(<new middleware>), is the super call necessary? It looks to resolve to the same thing

Simplified!

lauzadis · 2025-01-17T15:15:11Z

...e/amazon/smithy/kotlin/codegen/rendering/endpoints/discovery/EndpointDiscoveryIntegration.kt


-    override fun enabledForService(model: Model, settings: KotlinSettings): Boolean =
-        model.expectShape<ServiceShape>(settings.service).hasTrait<ClientEndpointDiscoveryTrait>()
+    override fun enabledForService(model: Model, settings: KotlinSettings): Boolean = isEnabledFor(model, settings)


Instead of repeating the integration's isEnabled check, can this just be true?

What do you mean by "repeating the integration's isEnabled check"? This method is the primary way the integration declares whether it's relevant for a particular service. If this were set to true then every service would get an endpointDiscoverer config parameter, endpoint error middleware, etc.

Sorry I got this mixed up thinking it was the middleware's isEnabledFor function, you can ignore this

lauzadis · 2025-01-17T15:23:28Z

...are/amazon/smithy/kotlin/codegen/rendering/endpoints/discovery/EndpointDiscoveryTestUtils.kt

+import software.amazon.smithy.kotlin.codegen.test.toSmithyModel
+
+fun model() =
+    // language=smithy


Does this give you proper syntax highlighting in the IDE? Doesn't seem to be working for me

Is this just for our reference?

No it's a feature of IntelliJ. We use it in other places: https://www.jetbrains.com/help/idea/using-language-injections.html#use-language-injection-comments

It does (mostly) work for me. The IDE can't seem to find the Smithy prelude so it doesn't know what any of the traits mean but keywords and language constructs appear nicely formatted:

Note that Smithy syntax support is provided by the Smithy plugin for IntelliJ. If you don't have that installed in your IDE, it'll probably look like plain text.

lauzadis · 2025-01-17T15:28:40Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/collections/ExpiringKeyedCache.kt

+ * [invalidate] methods are `suspend` functions to allow for cross-context synchronization and potentially-expensive
+ * value lookup.
+ *
+ * Values in the cache _may_ expire and are retrieved as [ExpiringValue]. When a value absent/expired in the cache,


nit/typo: "When a value is absent/expired"

Good catch!

lauzadis · 2025-01-17T15:29:39Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/collections/ExpiringKeyedCache.kt

+     * exceptions, fall back to other caches, etc.
+     * @param key The key for which to look up a value
+     * @param valueLookup A possibly-suspending function which returns the read-through value associated with a given
+     * key. This function is invoked when the cache, for a given key, does not contain a value or the value is expired.


nit: "for a given key" -> "for the given key"

Right you are!

0marperez · 2025-01-17T16:43:03Z

...mithy/kotlin/codegen/rendering/endpoints/discovery/DefaultEndpointDiscovererGeneratorTest.kt

+class DefaultEndpointDiscovererGeneratorTest {
+    @Test
+    fun testClass() {
+        val actual = render()


Question: Can the render function be called once at the class level instead of for each test? Same question applies to the EndpointDiscovererInterfaceGeneratorTest

Yes, it can. Refactored.

0marperez · 2025-01-17T16:50:05Z

...are/amazon/smithy/kotlin/codegen/rendering/endpoints/discovery/EndpointDiscoveryTestUtils.kt

+import software.amazon.smithy.kotlin.codegen.test.toSmithyModel
+
+fun model() =
+    // language=smithy


Is this just for our reference?

github-actions · 2025-01-17T18:08:35Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
http-client-jvm.jar	318,966	317,120	1,846	0.58%
runtime-core-jvm.jar	819,667	818,677	990	0.12%
aws-signing-common-jvm.jar	71,184	71,597	-413	-0.58%

fix: overhaul endpoint discovery to favor endpointUrl when provided a…

af8c49d

…nd to make endpoint discoverers interfaces so custom implementations can be provided

ianbotsf added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Jan 15, 2025

This comment has been minimized.

Sign in to view

ianbotsf mentioned this pull request Jan 16, 2025

fix: overhaul endpoint discovery aws/aws-sdk-kotlin#1506

Merged

Merge remote-tracking branch 'origin/v1.5-main' into fix-overhaul-ep-…

c3e8597

…discovery

This comment has been minimized.

Sign in to view

retrigger CI

16292b4

This comment has been minimized.

Sign in to view

ianbotsf marked this pull request as ready for review January 16, 2025 21:37

ianbotsf requested a review from a team as a code owner January 16, 2025 21:37

lauzadis approved these changes Jan 17, 2025

View reviewed changes

0marperez approved these changes Jan 17, 2025

View reviewed changes

clean up miscellaneous grammar, naming, and code factoring issues

62e2f1d

ianbotsf merged commit 2e44922 into v1.5-main Jan 17, 2025
16 checks passed

ianbotsf deleted the fix-overhaul-ep-discovery branch January 17, 2025 19:37

ianbotsf mentioned this pull request Feb 13, 2025

fix: favor endpointUrl over endpoint discovery when provided #1240

Merged

fix: overhaul endpoint discovery #1221

fix: overhaul endpoint discovery #1221

Uh oh!

Conversation

ianbotsf commented Jan 15, 2025

Issue #

Description of changes

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 17, 2025

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants