Skip to content

refactor(server): split ktor config into separate files #1816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

LeoColman
Copy link
Member

@LeoColman LeoColman commented Feb 10, 2025

This PR prepares the ground to add metrics by refactoring the code to enhance metric-attachment-points.

import io.github.typesafegithub.workflows.actionbindinggenerator.domain.ActionCoords
import io.ktor.http.Parameters

fun ActionCoords(
Copy link
Member Author

Choose a reason for hiding this comment

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

Used both in artifact and metadata

val nameAndPath = parameters["name"]!!.split("__")
val name = nameAndPath.first()
val path = nameAndPath.drop(1).joinToString("/").takeUnless { it.isBlank() }
val version = if (extractVersion) parameters["version"]!! else "irrelevant"
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the only difference between the two constructions of ActionCoords


val bindingsCache = Cache.Builder<ActionCoords, ArtifactResult>().expireAfterWrite(1.hours).build()

fun Routing.artifactRoute() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted from the main file, with minor 'flavor' changes. No functionality should have changed

Comment on lines 9 to 11
get("/metrics") {
call.respondText(prometheusRegistry.scrape())
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Required for prometheus to get the metrics


private val deliverOnRefreshRoute = System.getenv("GWKT_DELIVER_ON_REFRESH").toBoolean()
suspend fun ApplicationCall.respondNotFound() = respondText("Not found", status = HttpStatusCode.NotFound)
Copy link
Member Author

Choose a reason for hiding this comment

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

Function used both in artifact and metadata. Couldn't find a better place to put it

import io.ktor.server.routing.get
import io.ktor.server.routing.route

fun Routing.metadataRoute() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Extracted from the main file, with minor 'flavor' changes. No functionality should have changed

Copy link
Member Author

Choose a reason for hiding this comment

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

Completely removed and replaced with Micrometer

metadata(refresh = true)
}
}
routing {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR 😄 This one mixes several kinds of changes (also unrelated to what you're after originally, so switching to Micrometer), so let's take it bit by bit.

The first change I propose to put into a separate PR is moving ktor-related routes and plugins config into separate file(s). I've always wanted to do it, especially when the endpoints got a bit more complicated, but somehow didn't get there. Let's put into a separate PR that just does this refactor and nothing else, so that it's easy to review and minimizes risk of regressions.

Once we have that merged, let's see what's left in this PR, probably will need to extract some more. Alternatively, I'm totally fine if you cannot afford iterating this way, and just want to use Micrometer - then, please jsut send a PR that introduces minimal changes to achieve it, without any larger refactors to not blur the diff.

@LeoColman LeoColman force-pushed the leocolman-patch branch 2 times, most recently from 8b76824 to 0c9d461 Compare February 12, 2025 10:51
@krzema12 krzema12 changed the title refactor(server): Restructure JitBindingServer refactor(server): split ktor config into separate files Feb 12, 2025
@krzema12 krzema12 merged commit e4349d1 into typesafegithub:main Feb 12, 2025
14 checks passed
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.

2 participants