When writing a ktor feature, under what circumstances should the feature add a phase vs reusing an e...

# ktor

[mp]

When writing a ktor feature, under what circumstances should the feature add a phase vs reusing an existing one?

[Deactivated User]

if you want to fine control the execution order or thinking about extensibility/semantics

[mp]

I'm not sure how to interpret that 🙂 Example: CallLogging feature defines its own phase, but existing Monitoring phase says:

Phase for tracing calls, useful for logging, ...

anyway, what I'm doing is writing a feature to grab request metadata and put it in a ThreadLocal so that it can be provided to Google Cloud's logging as metadata for each log call (https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#HttpRequest)

[Deactivated User]

do you have link? maybe it is wrong

[mp]

As part of this, it'd be nice to be support `XForwardedHeaderSupport`'s handling of XForwardedFor so it shows the real client IP, not just the load balancer. That feature intercepts the Features phase.

[mp]

https://github.com/ktorio/ktor/blob/master/ktor-server/ktor-server-core/src/io/ktor/features/CallLogging.kt#L108

[mp]

vs https://github.com/ktorio/ktor/blob/master/ktor-server/ktor-server-core/src/io/ktor/application/ApplicationCallPipeline.kt#L33

[mp]

At first I cargo-cult copied CallLogging, but now I'm not sure what I should do. I could:
- Intercept Monitoring because that seems like the best generic fit as per docs
- Intercept Features and document that users should apply XForwardedHeaderSupport first if they want that data to be included in the logging HttpRequest metadata
- Make a custom phase and insert it after Features
- Make a custom phase and insert it after a user-configured phase

[Deactivated User]

I would go for intercepting Monitoring unless you need an specific order of execution.

But I’m not 100% sure about why in this case it was inserted before. I would say that it was so that the logging of the call, occurs before any other kind of monitoring or logging to give some predictability. But I can’t tell about the rationale of that decision, why for example wasn’t configured to be executed after instead.

Maybe @cy can give more information about that

[mp]

if I intercept Monitoring, that would mean I couldn't get the origin info from XForwardedHeaderSupport right?

[Deactivated User]

yeah, I think so

[mp]

OK. So, Features then I guess, and document to put it after XFHS if the consumer wants that data

[mp]

(that's what I'm currently doing now in testing and it Seems To Work (TM))

[Deactivated User]

Maybe XForwardedHeaderSupport should be put earlier in the chain, since it contributes to construct the request? Not sure.

[mp]

It does seem like maybe it's a bit late since it changes the meaning of some request elements

[mp]

Just based on the name & docs, there's a case to be made that it should be in Setup?

[mp]

You wouldn't really want call.request.origin to change its meaning after Monitoring since presumably your monitoring features care about that...

[mp]

Or maybe it should be in its own phase before Monitoring.

[Deactivated User]

I would personally put it in Setup, but might be missing things, so let’s wait for @cy: quick summary so you don’t need to read everything:

- An interceptor at the Monitoring phase might require origin information generated contributed by XForwardedHeaderSupport
- We were discussing if it might have sense to move XForwardedHeaderSupport eariler in the chain. Like at the Setup phase.

What do you think?

[cy]

What exactly are you trying to achieve ?

[Deactivated User]

anyway, what I’m doing is writing a feature to grab request metadata and put it in a ThreadLocal so that it can be provided to Google Cloud’s logging as metadata for each log call (https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry#HttpRequest)

[cy]

Logging is installed before forwarded headers so you can always see actual request headers before any features

[Deactivated User]

As part of this, it’d be nice to be support `XForwardedHeaderSupport`’s handling of XForwardedFor so it shows the real client IP, not just the load balancer. That feature intercepts the Features phase.

[Deactivated User]

he wants to access the original user ip

[cy]

I see the point but I still don't like the idea to install features into Setup phase

[Deactivated User]

aha, @mp then maybe you can continue doing it like you are doing it now

[cy]

It is intended to prepare environment for very special cases

[mp]

Sure, I can understand not wanting to tamper with Setup. Just seems that maybe Monitoring stuff would want the end user's IP available (as I do in my case)

[cy]

Your usecase is not exactly clear, are you trying to log user ips? Perhaps we need to make some `XForwardedHeaderSupport`’s functions public so you can reuse logic instead of moving it to an earlier phase

[mp]

Google's cloud logging thing has special support for HTTP request metadata (and some response stuff but that's not really waht I'm worried about, like status code). I'd like to populate it automatically for any logging calls that happen in a coroutine context that's for a ktor call.

[mp]

so, I have a feature that jams stuff in a threadlocal so that it can be accessed in plain-java logging stuff

[mp]

I have an almost-working prototype; I'll publish it shortly if you want to take a look

[mp]

Anyway, as you can imagine, it would be nice if the requestor IP i was adding to logging was the actual client, not just the load balancer's IP every time

[cy]

please note that thread locals are not guaranteed to work without coroutine's ThreadContextElements

[mp]

I don't have a strong opinion for my particular use case about relying on XFHS to have already done that for me for call.request.origin or if I call some public method, but it does seem like if a user installs XFHS then they could reasonably expect that stuff in Monitoring that uses call.request.origin would see the value that XFHS has set

[cy]

As a coroutine could migrate between threads after suspension

[mp]

Yes, I'm using a ThreadContextElement (via asContextElement)

[cy]

Do you really need to have access to origin so early?

[mp]

I'm not sure on what the best practices are, so... maybe? 🙂

[mp]

I want to do the thing I described above. Given the docs on the Monitoring phase, it seems that that's the phase it should live in, but if I intercept there, I won't get `XFHS`'s origin.

[mp]

Because feature authors can always use request.local instead of request.origin, I don't think there's a downside to moving XFHS earlier?

[cy]

Is it too late to do your stuff in a phase after Features?

[mp]

It would mean that any logging that is done inside Features wouldn't get the metadata attached, which seems like a shame

[mp]

it's not a big deal in my current use case but it seems like a nice thing to have, right?

[cy]

CallId feature would like to log ip's as well... I see your point

[cy]

Let me think about it

[cy]

Could you please open an issue on GH ?

[mp]

Sure. For now I have a workaround (install in Features, and have it be the last feature) but it does seem there is a design question lurking around in here

[mp]

yep, I plan on using CallLogging + CallId to put the call id in MDC, and then doing more decoration of GCP's LogEvent to insert MDC.

[cy]

Sure, and since we are approaching 1.0.0 soon it is urgent to decide or mark this part as experimental to warn users about the problem

[mp]

OK. I'll get to it within an hour.