When writing a ktor feature, under what circumstan...
# ktorm
mp
10/29/2018, 1:38 PMWhen writing a ktor feature, under what circumstances should the feature add a phase vs reusing an existing one?
d
Deactivated User
10/29/2018, 1:39 PMif you want to fine control the execution order or thinking about extensibility/semantics
m
mp
10/29/2018, 1:41 PMI'm not sure how to interpret that 🙂 Example: CallLogging feature defines its own phase, but existing
MonitoringPhase 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)
d
Deactivated User
10/29/2018, 1:42 PMdo you have link? maybe it is wrong
m
mp
10/29/2018, 1:42 PMAs 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
Featuresmp
10/29/2018, 1:45 PMAt first I cargo-cult copied CallLogging, but now I'm not sure what I should do. I could:
- Intercept
MonitoringFeaturesXForwardedHeaderSupportHttpRequestFeatures👀 1
d
Deactivated User
10/29/2018, 2:13 PMI 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
m
mp
10/29/2018, 2:19 PMif I intercept Monitoring, that would mean I couldn't get the origin info from
XForwardedHeaderSupport👀 1
d
Deactivated User
10/29/2018, 2:25 PMyeah, I think so
m
mp
10/29/2018, 2:25 PMOK. So,
FeaturesXFHSmp
10/29/2018, 2:25 PM(that's what I'm currently doing now in testing and it Seems To Work (TM))
d
Deactivated User
10/29/2018, 2:26 PMMaybe
XForwardedHeaderSupportm
mp
10/29/2018, 2:27 PMIt does seem like maybe it's a bit late since it changes the meaning of some request elements
mp
10/29/2018, 2:27 PMJust based on the name & docs, there's a case to be made that it should be in Setup?
mp
10/29/2018, 2:28 PMYou wouldn't really want
call.request.originMonitoringmp
10/29/2018, 2:28 PMOr maybe it should be in its own phase before Monitoring.
d
Deactivated User
10/29/2018, 2:31 PMI 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
MonitoringoriginXForwardedHeaderSupportXForwardedHeaderSupportSetupc
cy
10/29/2018, 2:35 PMWhat exactly are you trying to achieve ?
d
Deactivated User
10/29/2018, 2:36 PManyway, 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)
c
cy
10/29/2018, 2:37 PMLogging is installed before forwarded headers so you can always see actual request headers before any features
d
Deactivated User
10/29/2018, 2:38 PMAs 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 thephase.Features
Deactivated User
10/29/2018, 2:38 PMhe wants to access the original user ip
c
cy
10/29/2018, 2:39 PMI see the point but I still don't like the idea to install features into Setup phase
d
Deactivated User
10/29/2018, 2:40 PMaha, @mp then maybe you can continue doing it like you are doing it now
c
cy
10/29/2018, 2:41 PMIt is intended to prepare environment for very special cases
m
mp
10/29/2018, 2:42 PMSure, I can understand not wanting to tamper with
SetupMonitoringc
cy
10/29/2018, 2:42 PMYour 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
➕ 1
m
mp
10/29/2018, 2:43 PMGoogle'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
10/29/2018, 2:43 PMso, I have a feature that jams stuff in a threadlocal so that it can be accessed in plain-java logging stuff
mp
10/29/2018, 2:43 PMI have an almost-working prototype; I'll publish it shortly if you want to take a look
mp
10/29/2018, 2:44 PMAnyway, 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
c
cy
10/29/2018, 2:44 PMplease note that thread locals are not guaranteed to work without coroutine's ThreadContextElements
m
mp
10/29/2018, 2:45 PMI don't have a strong opinion for my particular use case about relying on
XFHScall.request.originXFHSMonitoringcall.request.originXFHSc
cy
10/29/2018, 2:45 PMAs a coroutine could migrate between threads after suspension
m
mp
10/29/2018, 2:45 PMYes, I'm using a ThreadContextElement (via
asContextElementc
cy
10/29/2018, 2:45 PMDo you really need to have access to
originm
mp
10/29/2018, 2:46 PMI'm not sure on what the best practices are, so... maybe? 🙂
mp
10/29/2018, 2:46 PMI want to do the thing I described above. Given the docs on the
Monitoringoriginmp
10/29/2018, 2:47 PMBecause feature authors can always use
request.localrequest.originXFHSc
cy
10/29/2018, 2:50 PMIs it too late to do your stuff in a phase after Features?
m
mp
10/29/2018, 2:52 PMIt would mean that any logging that is done inside Features wouldn't get the metadata attached, which seems like a shame
mp
10/29/2018, 2:52 PMit's not a big deal in my current use case but it seems like a nice thing to have, right?
c
cy
10/29/2018, 2:56 PMCallId feature would like to log ip's as well... I see your point
cy
10/29/2018, 2:56 PMLet me think about it
cy
10/29/2018, 2:57 PMCould you please open an issue on GH ?
m
mp
10/29/2018, 2:57 PMSure. 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
10/29/2018, 2:59 PMyep, 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.
c
cy
10/29/2018, 2:59 PMSure, 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
m
mp
10/29/2018, 3:00 PMOK. I'll get to it within an hour.
3 Views