-
-
Notifications
You must be signed in to change notification settings - Fork 584
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
PoC: feat(middleware/hook): introduce hook middleware #3025
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3025 +/- ##
==========================================
- Coverage 94.76% 94.31% -0.46%
==========================================
Files 136 137 +1
Lines 13369 13435 +66
Branches 2264 2297 +33
==========================================
+ Hits 12669 12671 +2
- Misses 700 764 +64 ☔ View full report in Codecov by Sentry. |
Hey @Code-Hex! It is still in draft stage and I am not even sure if I will do it or not, but I would like to know your opinion if you like. |
Updated 36267e2 |
Quite Interesting! Some thoughts I have regarding the API -
app.use(
hook(async (c, handler) => {
console.log("Before")
await handler();
console.log("After")
})
) These are just my viewpoints, correct me if I am wrong but I love the idea of a function running between middleware, already thinking about a lot of stuff to build with it 😂 |
Thank you. I understand your argument for more simplicity. But if you write the following, the app.use(
hook(async (c, handler) => {
console.log("Before")
await handler();
console.log("After")
})
) Why beforeNext / afterNextBeforeNext and afterNext are provided to allow the processing time of app.use(async (c, next) => {
// before hook
hardWorkBeforeNext()
// beforeNext hook
await next()
// afterNext hook
hardWorkAfterNext()
// after hook
}) |
@usualoma Thank you very much for your prompt creation of a PoC based on the ideas you presented at the Hono Conference! The image is very close. The idea originally came from my comment that it would be nice to have a mechanism in place to help Hono users visualize how their applications are being used. The idea on which this idea is based is
type RPCStats interface {
// IsClient returns true if this RPCStats is from client side.
IsClient() bool
} The reason 8 implementations are provided to satisfy this interface. They are Using these, users can create a HandleRPC in this way to collect statistical information. // HandleRPC implements per RPC tracing and stats implementation.
func (h *serverStatsHandler) HandleRPC(ctx context.Context, rs stats.RPCStats) {
ri := getRPCInfo(ctx)
if ri == nil {
logger.Error("ctx passed into server side stats handler metrics event handling has no server call data present")
return
}
h.processRPCData(ctx, rs, ri.ai)
}
func (h *serverStatsHandler) processRPCData(ctx context.Context, s stats.RPCStats, ai *attemptInfo) {
switch st := s.(type) {
case *stats.InHeader:
if ai.pluginOptionLabels == nil && h.options.MetricsOptions.pluginOption != nil {
labels := h.options.MetricsOptions.pluginOption.GetLabels(st.Header)
if labels == nil {
labels = map[string]string{} // Shouldn't return a nil map. Make it empty if so to ignore future Get Calls for this Attempt.
}
ai.pluginOptionLabels = labels
}
h.serverMetrics.callStarted.Add(ctx, 1, otelmetric.WithAttributes(otelattribute.String("grpc.method", ai.method)))
case *stats.OutPayload:
atomic.AddInt64(&ai.sentCompressedBytes, int64(st.CompressedLength))
case *stats.InPayload:
atomic.AddInt64(&ai.recvCompressedBytes, int64(st.CompressedLength))
case *stats.End:
h.processRPCEnd(ctx, ai, st)
default:
}
} If these handlers were actually passed as options inside the framework, they would be called this way. func (s *Server) processUnaryRPC(ctx context.Context, t transport.ServerTransport, stream *transport.Stream, info *serviceInfo, md *MethodDesc, trInfo *traceInfo) (err error) {
shs := s.opts.statsHandlers
if len(shs) != 0 || trInfo != nil || channelz.IsOn() {
// ...
var statsBegin *stats.Begin
for _, sh := range shs {
beginTime := time.Now()
statsBegin = &stats.Begin{
BeginTime: beginTime,
IsClientStream: false,
IsServerStream: false,
}
sh.HandleRPC(ctx, statsBegin)
}
}
// Processing to call handler
// after called
for _, sh := range shs {
end := &stats.End{
BeginTime: statsBegin.BeginTime,
EndTime: time.Now(),
}
if err != nil && err != io.EOF {
end.Error = toRPCErr(err)
}
sh.HandleRPC(ctx, end)
} In the same file, InPayload is called immediately after parsing the request body and OutPayload is called immediately after writing the response body. The other InHeader, OutHeader, InTrailer, and OutTrailer call https://github.com/grpc/grpc-go/blob/f199062ef31ddda54152e1ca5e3d15fb63903dc3/server.go#L1206-L1246 Based on the above, I think that Since these are hooks for monitoring, they are defined on the assumption that no errors are thrown. And I do not believe they should be used to hack complex business logic. In other words, I believe this should be available as something completely different from existing middleware. I personally thought it would be nice if we could pass it on as an option for Hono rather than as middleware. stats({
tagHandler(c, handlerInfo) { // tagRouter is better??
// We can handle w/ handler information or inject to HonoContext
// We may have router information at here??
},
handleHandler(c, handlerStats) {
// Type switch and handle them
},
}) |
Thank you. That is very helpful information. I understand that "grpc" provides the following eight,
What would a generic framework, such as Hono, be able to provide? I still don't understand, when should Is there anything we can do by 'providing it as a core feature rather than middleware'? |
@usualoma Thanks for reply me. I'm writing just focus for
It is right after the request headers are received and right after the response headers to be returned are finalized. Since multiple middleware can modify the request and response, I believe that only the core functionality of the framework can retrieve the finalized information.
Payload is a concern of mine as well. Hono has a mechanism to validate payloads such as path parameters and form bodies by using validator as middleware. I thought that
Personally, I consider the Trailer to be optional. gRPC uses it to express Response Status details, so it is important, but the HTTP Web Framework in general does not consider it that important. |
Usage
Output
The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code