Add the resource to client requests when using the Resources Plugin.
#4747
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (3)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt(17 hunks)ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (1)
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt (1) (1)
urlTemplate(314-333)
🔇 Additional comments (12)
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (9)
156-157: Good resource definition for testing optional query parameters.The new
PathWithOptionalQueryParameterclass is well structured with a requiredidparameter and an optionalqueryparameter, which is perfect for testing the URL template generation with optional query parameters.
159-183: Test verifies URL template attribute is correctly added.This test effectively verifies that the
URL_TEMPLATEattribute is correctly set during requests with resources. The approach of using a client plugin to assert the attribute value is a clean way to validate the functionality.
185-191: Good test for URL template generation with required parameters.This test ensures that the URL template correctly generates the expected format for resources with multiple required parameters, properly handling the path structure and query parameters.
193-199: Test ensures optional query parameters are correctly marked in templates.This test confirms that optional query parameters (marked with nullable types or default values) are correctly represented in the URL template with a
?suffix, which aligns with the PR objectives.
6-6: Appropriate imports added for new functionality.The imports for
createClientPlugin,ResourcesFormat, andserializerare correctly added to support the new URL template functionality being tested.Also applies to: 14-15
156-157: Well-designed test resource with optional parameter.This resource class is properly defined with an optional query parameter, making it suitable for testing URL template generation with optional parameters. The design aligns well with the PR objective of supporting template URL attributes.
159-183: Good verification of URL template attributes.This test effectively verifies that the URL template is correctly added as an attribute to the request when using the Resources plugin. The implementation properly:
- Sets up a test environment with the Resources plugin
- Creates a client plugin to assert the URL template matches the expected value
- Verifies the template is available via the
URL_TEMPLATEattributeThis directly addresses the core goal of the PR to "add template URL attribute to client requests."
185-191: Good test for URL template generation with path and query parameters.This test properly validates the
urlTemplatefunction's output for a more complex resource type with nested path parameters and a list query parameter. The expected template format correctly follows the pattern described in the PR objectives.
193-199: Effective verification of optional query parameter handling.This test confirms that optional query parameters are properly marked with a question mark in the URL template. The test case complements the previous one by specifically validating how optional parameters are denoted in the template.
This functionality is crucial for downstream plugins that will use these templates for request introspection and span naming.
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt (3)
305-306: LGTM! Good definition of the URL template attribute key.The
URL_TEMPLATEattribute key is appropriately defined as a public constant, making it accessible to downstream plugins that need to read this information from requests.
307-312: Well-designed helper function for setting URL template attribute.This function encapsulates the logic for generating and storing a URL template in the request attributes, providing a clean API for the HTTP method implementations to use.
314-333: URL template generation function handles both path and query parameters appropriately.The
urlTemplatefunction correctly builds the URL template by combining:
- The path pattern from the resource format
- Query parameters with proper formatting for required and optional parameters
The handling of optional parameters with the
?suffix aligns with common template syntax conventions and ensures downstream plugins can properly understand the template structure.
...client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt
Outdated
Show resolved
Hide resolved
MarcusDunn
force-pushed
the
ad-template-to-request
branch
from
3fdc376 to
881f666
Compare
MarcusDunn
changed the title
Resources Plugin.Resources Plugin.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt (1)
305-309: Fix minor grammatical error in documentationThere's a small grammatical error in the documentation for the RESOURCE attribute.
-/** - * The instance of the [Resource] annotated class used to for a request. - * Plugins may want to utilize this for monitoring. - */ +/** + * The instance of the [Resource] annotated class used for a request. + * Plugins may want to utilize this for monitoring. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt(17 hunks)ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt(3 hunks)
🔇 Additional comments (5)
ktor-client/ktor-client-plugins/ktor-client-resources/common/src/io/ktor/client/plugins/resources/builders.kt (2)
12-14: Good addition of necessary importsThese imports are correctly added to support the new functionality for storing resource instances as attributes in HTTP requests.
44-44: Well-implemented attribute storage across all HTTP methodsThe consistent addition of
attributes.put(RESOURCE, resource)across all HTTP methods ensures that the resource instance is always accessible during request execution. This implementation aligns perfectly with the PR objective to enhance introspection capabilities.Also applies to: 61-61, 78-78, 95-95, 112-112, 129-129, 146-146, 163-163, 180-180, 197-197, 214-214, 231-231, 248-248, 265-265, 282-282, 299-299
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (3)
6-6: Appropriate imports addedThe imports for
createClientPluginandassertSameare correctly added to support the new test functionality.Also applies to: 16-16
155-156: Well-defined test resource classThis resource class is well-designed for testing the URL template attribute functionality, with an ID parameter in the path and an optional query parameter.
158-181: Comprehensive test for resource attribute storageThe test effectively verifies that the resource instance is correctly stored in the request attributes. The use of
assertSameensures object identity is maintained, and the client plugin approach elegantly hooks into the request pipeline for verification.This test is aligned with the PR objective of enhancing the introspection capabilities for downstream plugins.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (1)
159-183: Good test implementation for RESOURCE attribute verification.This test effectively verifies the PR's main objective: ensuring that resource instances are correctly associated with request attributes. The test:
- Creates a resource instance
- Uses a custom plugin to verify the resource is stored in the request attributes
- Confirms the request completes successfully
The use of
assertSameis appropriate for verifying object identity rather than just equality.For better documentation, consider adding a comment explaining that this test verifies the behavior introduced by the
RESOURCEattribute key for tracking template resources in requests.@Test -fun `check template is added as attribute`() = testWithEngine(MockEngine) { +/** + * Verifies that the resource instance is correctly associated with the request + * attributes via the RESOURCE attribute key, which enables downstream plugins + * to introspect the template used for creating request URLs. + */ +fun `check template is added as attribute`() = testWithEngine(MockEngine) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt(3 hunks)
🔇 Additional comments (2)
ktor-client/ktor-client-plugins/ktor-client-resources/common/test/ResourcesTest.kt (2)
6-6: Good addition of necessary imports.The imports for
createClientPluginandassertSameare correctly added to support the new test functionality.Also applies to: 16-16
155-157: Well-structured resource class definition.The
PathWithOptionalQueryParameterclass is cleanly defined with an appropriate path template and properly typed parameters. The optional query parameter with a default value ofnullis a good choice for testing the attribute functionality.
MarcusDunn
force-pushed
the
ad-template-to-request
branch
from
54a01a1 to
8cd1665
Compare
…o do more than just get the template.
MarcusDunn
force-pushed
the
ad-template-to-request
branch
from
8cd1665 to
f6fe069
Compare
Subsystem
Client, Resources
Motivation
This PR allows downstream plugins to introspect the resource used to create the request URL.
The motivating use case for this is to allow spans to be named
GET api/user/{id}?query={query}instead ofGET, which is the best downstream telemetry can do right now.This would result in a huge quality of life improvement around aggregating client requests to the same endpoint. Currently I'm writing regex against the full URL which is both slow, error prone and has to be done for each endpoint.
Solution
I've added a
RESOURCEattribute to the request when the resources plugin is used.I've made a seperate PR to opentelemetry-java-instrumentation to use this attribute to populateurl.templateopen-telemetry/opentelemetry-java-instrumentation#13572I will make a PR to opentelemetry-java-instrumentation once this is merged so I can test on that side. (as requested by their maintainers)
Alternatives