Skip to content
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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

usualoma
Copy link
Member

@usualoma usualoma commented Jun 23, 2024

Usage

import { Hono } from 'hono'
import { hook } from 'hook'

const app = new Hono()
app.use(
  hook({
    before(c, handler, handlerContext, traceId) {
      console.log(traceId, 'before', handler.name)
      handlerContext['start'] = Date.now()
    },
    beforeNext(c, handler, _, traceId) {
      console.log(traceId, 'beforeNext', handler.name)
    },
    afterNext(c, handler, _, traceId) {
      console.log(traceId, 'afterNext', handler.name)
    },
    after(c, handler, handlerContext, traceId) {
      const elapsed = Date.now() - handlerContext['start']
      console.log(traceId, `after ${handler.name} ${elapsed}ms`, handler.name)
    },
  })
)

app.use(async function middlewareA(c, next) {
  await new Promise((resolve) => setTimeout(resolve, 100))
  await next()
})

app.use(async function middlewareB(c, next) {
  await new Promise((resolve) => setTimeout(resolve, 200))
  await next()
})

app.get('/', async function handler(c) {
  await new Promise((resolve) => setTimeout(resolve, 200))
  return c.text('Hello, World!')
})

export default app

Output

c0934c4a53236 before middlewareA
c0934c4a53236 beforeNext middlewareA
c0934c4a53236 before middlewareB
c0934c4a53236 beforeNext middlewareB
c0934c4a53236 before handler
c0934c4a53236 after handler 203ms handler
c0934c4a53236 afterNext middlewareB
c0934c4a53236 after middlewareB 406ms middlewareB
c0934c4a53236 afterNext middlewareA
c0934c4a53236 after middlewareA 508ms middlewareA

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 3.03030% with 64 lines in your changes missing coverage. Please review.

Project coverage is 94.31%. Comparing base (60d68ca) to head (36267e2).

Files Patch % Lines
src/middleware/hook/index.ts 0.00% 62 Missing ⚠️
src/request.ts 50.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@usualoma
Copy link
Member Author

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.
Would your expectations of middleware traceability be achieved with such a middlewara?

@usualoma
Copy link
Member Author

Updated 36267e2

@MathurAditya724
Copy link
Contributor

Quite Interesting! Some thoughts I have regarding the API -

  1. Isn't afterNext and after functions getting called one after the other, so to simplify can't we just have an after function?
  2. Not sure about beforeNext as I don't like some random code running between my code which might alter the response. I believe beforeNext func and before func of the next middleware will have the same params, so what's the point of beforeNext?
  3. Can we simplify the hook API and make it similar to the middleware function?
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 😂

@usualoma
Copy link
Member Author

@MathurAditya724

Thank you. I understand your argument for more simplicity.

But if you write the following, the handler can't be a handler as it is in the original. It becomes a wrapped function. Here we have to ask, "What function are you hooking?" so we need to know, so we need to pass a bit more information.

app.use(
  hook(async (c, handler) => {
    console.log("Before")
    await handler();
    console.log("After")
  })
)

Why beforeNext / afterNext

BeforeNext and afterNext are provided to allow the processing time of hardWorkBeforeNext() and hardWorkAfterNext() to be measured in the following code.

app.use(async (c, next) => {

  // before hook

  hardWorkBeforeNext()

  // beforeNext hook

  await next()

  // afterNext hook

  hardWorkAfterNext()

  // after hook

})

@Code-Hex
Copy link
Contributor

Code-Hex commented Jun 30, 2024

@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 stats.Handler in grpc-go. As the name implies, if we implement an object that satisfies the stats.Handler interface, you can aggregate the usage by the users themselves.

HandleRPC is the easiest to imagine. It takes an interface called RPCStats as a parameter.

type RPCStats interface {
	// IsClient returns true if this RPCStats is from client side.
	IsClient() bool
}

The reason IsClient is a method is because gRPC allows client and server middleware and stats Handler to use the same interface.

8 implementations are provided to satisfy this interface. They are Begin, InHeader, InPayload, InTrailer, OutHeader, OutPayload, OutTrailer, End.

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:
	}
}

https://github.com/grpc/grpc-go/blob/f199062ef31ddda54152e1ca5e3d15fb63903dc3/stats/opentelemetry/server_metrics.go#L203-L232

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 HandleRPC at similar times.

https://github.com/grpc/grpc-go/blob/f199062ef31ddda54152e1ca5e3d15fb63903dc3/server.go#L1206-L1246


Based on the above, I think that TagRPC (routing information) and HandleRPC (handling information) hooks are the parts that can be used for Hono. I think users can hook the middleware themselves if they work hard enough, so it may not be necessary to do so.

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
    },
})

@usualoma
Copy link
Member Author

usualoma commented Jul 1, 2024

@Code-Hex

Thank you. That is very helpful information.

I understand that "grpc" provides the following eight,

Begin, InHeader, InPayload, InTrailer, OutHeader, OutPayload, OutTrailer, End.

What would a generic framework, such as Hono, be able to provide? I still don't understand, when should handleHandler(c, handlerStats) be called?

Is there anything we can do by 'providing it as a core feature rather than middleware'?

@Code-Hex
Copy link
Contributor

Code-Hex commented Jul 3, 2024

@usualoma Thanks for reply me.

I'm writing just focus for handleHandler.

start and end are mandatory (metrics or trace usecase). I think this should be called immediately after Hono receives the request, and at the last layer when all processing is done and the response is returned.

Start, End

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.

Header

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. InPayload is exactly what I thought it would be nice to call c.req.json() or c.req.valid() when the request payload is successfully parsed.

I thought that OutPayload should be similar to the timing of End, a fixed endpoint where the response body is not changed any more.

Payload

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.

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.

3 participants