proposal: integrate with github.com/valyala/fasthttp

[wadrn]

Proposal:
as fasthttp based server is up to 10 times faster than net/http, if we should integrate fasthttp in the framework?

[gbbr]

Yes I think that would be fantastic! Would you be interested in doing the work? If yes, what's the easiest way to integrate with this library?

[vjfreestar]

Is anyone working on this project at this moment :)

[knusbaum]

@vjfreestar I don't believe anybody is working on this currently. Are you interested in working on it?

[ridhoperdana]

any update on this? I am stuck with the usage of context.Context on tracer package because fasthttp use its own context type.

[knusbaum]

@ridhoperdana This is still on our radar but is not being actively worked on. If you have questions about our use of context.Context we can provide you some guidance.

[tills13]

I think I'm running into the same issue as @ridhoperdana.

fasthttp uses it's own context-like interface (RequestCtx) for its handlers. In my case, I've setup a wrapper for these handlers that ideally would inject a new span into the context:

func WrapHandler(h fasthttp.RequestHandler, resourceName string) fasthttp.RequestHandler {
	return func(ctx *fasthttp.RequestCtx) {
		opts := []ddtrace.StartSpanOption{
			// ... (setup opts)
		}

		// read remote span from headers
		if spanCtx, err := tracer.Extract(tracer.HTTPHeadersCarrier(headers)); err == nil {
			opts = append(opts, tracer.ChildOf(spanCtx))
		}

		// this is the top-level span - ignore the ctx as we have our own RequestCtx
		span, parentCtx := tracer.StartSpanFromContext(ctx, "http.request", opts...)
		defer span.Finish()

		h(ctx) // <-- this ctx must be a ctx.RequestCtx, not a context.
	}
}

example usage:

server := fasthttp.Server{
  Handler: WrapHandler(func (ctx *fasthttp.RequestCtx) { ... }, "DoSomething"))
}

server.ListenAndServe("0.0.0.0:9000")

I believe I need to use parentCtx to really glue everything together, but downstream handlers (h) - again - require a fasthttp.RequestCtx not a context.Context

Now... I realize that that's not the only way to do it, but I believe the alternative is to include this boilerplate span setup in every handler individually. Downstream of the handlers, in my case, only require context.Contexts - RequestCtx "implements" (is that the right terminology in Go) that interface, so usage there is fine, it's just a matter of getting it into the context.

Let me know if you need any further information.

[ridhoperdana]

@knusbaum From what I see the context is used to help if someone wants to create a child span inside whatever function that used the same context, am I right?

@tills13 So you modified the RequestCtx of fasthttp to implement the context.Context interface?

Right now I am just ignoring the context.Context usage, by using a context.Background inside the handler, and omit the return of parentCtx. Somehow it still sends trace data to the Datadog dashboard (even though only 1 span).

[tills13]

@tills13 So you modified the RequestCtx of fasthttp to implement the context.Context interface?

no, RequestCtx already implements context.Context - if you look below here, you'll see that it implements all the methods required by context.Context. There's a test which ensures it implments context.Context.

The issue to me seems that you can use a RequestCtx as a context.Context but you cannot use a context.Context as a RequestCtx (for obvious reasons) and the hierarchy seems to be RequestCtx in the upper handler / wrapper -> context.Context from StartSpanFromContext -> RequestCtx in the actual handler. Further, AFAIK there should only ever be one RequestCtx - StartSpanFromContext for example creates a new context for every span. Simply overwriting the span in RequestCtx isn't sufficient either since, well, you're overwriting parent spans that are still active.

I've kinda got something to work... though I'm not sure how many Go rules I've violated here or if my assumptions about spans hold true.

The way I see it, your RequestCtx should map to exactly one span - that's the "root" span for your Go service (though it could have a remote span as a parent).

I've added two helper functions

func InjectParentSpan(ctx *fasthttp.RequestCtx, s tracer.Span) {
	if _, exists := ParentSpanFromContext(ctx); exists {
		panic(fmt.Errorf("InjectParentSpan should only be called once"))
	}

	ctx.SetUserValue(parentSpanKey, s)
}

// StartParentSpan starts a root span which will be injected into the RequestCtx
func StartParentSpan(ctx *fasthttp.RequestCtx, operationName string, opts ...tracer.StartSpanOption) tracer.Span {
	s := tracer.StartSpan(operationName, opts...)
	InjectParentSpan(ctx, s)

	return s
}

and updated my wrapper to use StartParentSpan

func WrapHandler(h fasthttp.RequestHandler, resourceName string) fasthttp.RequestHandler {
	return func(ctx *fasthttp.RequestCtx) {
		opts := []ddtrace.StartSpanOption{
			// ... (setup opts)
		}

		// read remote span from headers
		if spanCtx, err := tracer.Extract(headers); err == nil {
			opts = append(opts, tracer.ChildOf(spanCtx))
		}

		span := StartParentSpan(ctx, "http.request", opts...)
		defer span.Finish()

		h(ctx)
	}
}

Finally, I have a function that sits between by request handlers and my downstream data providers (DAOs, ElasticSearch, etc)

// MakeChildSpanContext extracts span from RequestCtx and returns a context.Context with the span associated
func MakeChildSpanContext(ctx *fasthttp.RequestCtx) context.Context {
	span, _ := ParentSpanFromContext(ctx)
	return tracer.ContextWithSpan(ctx, span)
}

In my handlers, I'll call that before passing the context down.

func SearchHandler(ctx *fasthttp.RequestCtx) {
	// ...

	searchResults, err := esClient.doSearch(MakeChildSpanContext(ctx), searchOptions)
}

[ridhoperdana]

@tills13 So you modified the RequestCtx of fasthttp to implement the context.Context interface?

no, RequestCtx already implements context.Context - if you look below here, you'll see that it implements all the methods required by context.Context. There's a test which ensures it implments context.Context.

The issue to me seems that you can use a RequestCtx as a context.Context but you cannot use a context.Context as a RequestCtx (for obvious reasons) and the hierarchy seems to be RequestCtx in the upper handler / wrapper -> context.Context from StartSpanFromContext -> RequestCtx in the actual handler. Further, AFAIK there should only ever be one RequestCtx - StartSpanFromContext for example creates a new context for every span. Simply overwriting the span in RequestCtx isn't sufficient either since, well, you're overwriting parent spans that are still active.

I've kinda got something to work... though I'm not sure how many Go rules I've violated here or if my assumptions about spans hold true.

The way I see it, your RequestCtx should map to exactly one span - that's the "root" span for your Go service (though it could have a remote span as a parent).

I've added two helper functions

func InjectParentSpan(ctx *fasthttp.RequestCtx, s tracer.Span) {
	if _, exists := ParentSpanFromContext(ctx); exists {
		panic(fmt.Errorf("InjectParentSpan should only be called once"))
	}

	ctx.SetUserValue(parentSpanKey, s)
}

// StartParentSpan starts a root span which will be injected into the RequestCtx
func StartParentSpan(ctx *fasthttp.RequestCtx, operationName string, opts ...tracer.StartSpanOption) tracer.Span {
	s := tracer.StartSpan(operationName, opts...)
	InjectParentSpan(ctx, s)

	return s
}

and updated my wrapper to use StartParentSpan

func WrapHandler(h fasthttp.RequestHandler, resourceName string) fasthttp.RequestHandler {
	return func(ctx *fasthttp.RequestCtx) {
		opts := []ddtrace.StartSpanOption{
			// ... (setup opts)
		}

		// read remote span from headers
		if spanCtx, err := tracer.Extract(headers); err == nil {
			opts = append(opts, tracer.ChildOf(spanCtx))
		}

		span := StartParentSpan(ctx, "http.request", opts...)
		defer span.Finish()

		h(ctx)
	}
}

Finally, I have a function that sits between by request handlers and my downstream data providers (DAOs, ElasticSearch, etc)

// MakeChildSpanContext extracts span from RequestCtx and returns a context.Context with the span associated
func MakeChildSpanContext(ctx *fasthttp.RequestCtx) context.Context {
	span, _ := ParentSpanFromContext(ctx)
	return tracer.ContextWithSpan(ctx, span)
}

In my handlers, I'll call that before passing the context down.

func SearchHandler(ctx *fasthttp.RequestCtx) {
	// ...

	searchResults, err := esClient.doSearch(MakeChildSpanContext(ctx), searchOptions)
}

Wow thanks for the detailed response, and how silly I am not knowing that the RequestCtx already implement context.Context.
Your solution to inject the span value inside the RequestCtx is a very good one. Maybe adding a helper to transfer it into standard context.Context will make it better, since I suppose we don't use fastHttp RequestCtx inside all our sub-function.

[tills13]

Maybe adding a helper to transfer it into standard context.Context will make it better,

That's what MakeChildSpanContext does.

since I suppose we don't use fastHttp RequestCtx inside all our sub-function

I'm not sure if it's always the case but it seems to me like a good idea to only really use RequestCtx in handlers but context.Context everywhere else since you can pass in a RequestCtx where a context.Context is required.

[fabiano-amaral]

Hello, have we any plans to implement this?

[knusbaum]

We have no plans to implement this in the immediate future, however we are interested in this integration.

If you would like this integration developed, there are two paths we can provide:

1. please contact support and let them know you want this feature. This will help get this work prioritized
2. contribute this yourself. There are several suggestions in this issue on how to implement this, but no consensus. If you want to contribute, we will work on a solution here, approve the proposal, and then work towards merging the implementation.

Until one of these things happen, this issue will be closed as stale.

[zarirhamza]

Because of current work being done a similar integration PR, we want to re-open this issue to keep fasthttp in our radar for potential near-future development.

The same offer still stands if anyone wants to create a rough draft PR or a starting point.