KTOR-8183 Add a simplified route string #4691
Conversation
ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingNode.kt
Outdated
Show resolved
Hide resolved
ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RoutingNode.kt
Show resolved
Hide resolved
|
Hey @adriandieter, thanks you for the PR. Nice start! Some minor things need to be fixed, and maybe we can avoid making a new API so it can go to the patch release. Could you please take a look? |
adriandieter
force-pushed
the
KTOR-8183-simplified-route-string
branch
from
c4be08d to
7d2709c
Compare
|
One point I was not sure about, is if we should set the new path property as default value for the metrics route label or keep the original behavior as default. |
adriandieter
changed the title
There was a problem hiding this comment.
@adriandieter, thank you for the PR! I have a couple of comments.
Please, cherry-pick the changes from this PR on top of 3.2.0-eap branch. It is required as this PR introduces new APIs. After that, please fix the issues found by linter and update API dump using the following command:
./gradlew :ktor-server:ktor-server-core:apiDump :ktor-server:ktor-server-plugins:ktor-server-metrics-micrometer:apiDump
ktor-server/ktor-server-core/common/test/io/ktor/server/routing/RouteTest.kt
Show resolved
Hide resolved
...tor-server-metrics-micrometer/jvm/src/io/ktor/server/metrics/micrometer/MicrometerMetrics.kt
Outdated
Show resolved
Hide resolved
...tor-server-metrics-micrometer/jvm/src/io/ktor/server/metrics/micrometer/MicrometerMetrics.kt
Outdated
Show resolved
Hide resolved
| @@ -119,6 +119,34 @@ public class MicrometerMetricsConfig { | |||
| public fun timers(block: Timer.Builder.(ApplicationCall, Throwable?) -> Unit) { | |||
| timerBuilder = block | |||
| } | |||
|
|
|||
| internal var transformRoute: RoutingNode.() -> String = { path } | |||
There was a problem hiding this comment.
Note: We should mark this issue as "impactful change" as it requires migration for those who relies on the old behavior
Uses RoutingNode.path as default label instead of RoutingNode.toString().
adriandieter
force-pushed
the
KTOR-8183-simplified-route-string
branch
from
7d2709c to
edf2d1b
Compare
adriandieter
force-pushed
the
KTOR-8183-simplified-route-string
branch
from
6d60bf3 to
abaec70
Compare
adriandieter
force-pushed
the
KTOR-8183-simplified-route-string
branch
from
abaec70 to
ec00f45
Compare
|
Thanks for the quick review @osipxd! Addressed all comments. |
| is PathSegmentConstantRouteSelector, | ||
| is PathSegmentParameterRouteSelector, | ||
| is PathSegmentOptionalParameterRouteSelector, | ||
| is PathSegmentTailcardRouteSelector, | ||
| is PathSegmentWildcardRouteSelector, | ||
| is PathSegmentRegexRouteSelector -> toString() |
There was a problem hiding this comment.
We could introduce a supertype PathSegmentRouteSelector for these selectors to make it possible to create custom route selectors appearing in path. This change should be backward compatible.
@bjhham, what do you think?
Subsystem
Server, server-core & metrics-micrometer
Motivation
KTOR-8183 Add a simplified route string
Solution
Add
pathextension property to RoutingNode. It returns a String representation of the path matched by the route. RoutingNodes not related to the path, i.e. from the authorization plugin or the http method are ommited here.Make the route label string of micrometer metrics configurable with the
transformRoutefunction inMicrometerMetricsConfigand useRoutingNode.pathas default.This fixes the issue of unwanted path elements, like
/(authenticate "default")/uri, in the metrics route label.The original behaviour can be configured by setting
transformRoute { parent.toString() }in the
MicrometerMetricsConfig.