Plan to Support //dd:span for More Diverse Function Signatures?

[korECM]

Currently, //dd:span only supports functions whose signatures include a context or an HTTP request. However, for functions that use frameworks like Echo (using echo.Context) or Fiber (using fiber.Ctx), manual tracing must still be added.

Do you have any plans to extend //dd:span support to include these function signatures as well?

func TraceMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
	//dd:span
    return func(c echo.Context) error {
        return next(c)
    }
}

if this improvement isn't too complex to implement, I would appreciate any guidance or ideas. If possible, I'd be happy to work on the changes and submit a PR.
Feel free to adjust the wording as needed!

[RomainMuller]

In principle, //dd:span supports any function; not just those accepting a context.Context or *net/http.Request parameter. That's been the case for a while.

In the example provided below, the pragma applies to the return statement; and that is not correctly picked up by the injector (I'm tempted to say this is a bug)... You could try the following to see if it works any better (the injector does crawl up assignment statements when looking for a pragma directive):

func TraceMiddleware(next echo.HanlderFunc) echo.HandlerFunc {
	//dd:span
	handler := func(c echo.Context) error {
		return next(c)
	}
	return handler
}

[RomainMuller]

I've created #540 to adjust the injector crawl for this situation; which I believe would fix this issue.

I need to write a couple of test cases to make sure this behaves as intended (in particular around arguments passed to function calls).

[korECM]

@RomainMuller
Thank you for reviewing the changes and for the quick fix!

If I understand correctly, //dd:span can be applied to any function—not just those with a ctx or an http request—and will eventually be extended further. But for now, it only supports ctx and http request, correct?

If so, may I work on adding support for the echo context as well? (This additional work is a high priority for me, though I realize it might not be as high a priority from the library maintainer's perspective, so I’m suggesting it cautiously.)

Assuming my understanding is correct, I believe we could modify this section as follows:

{{- $ctx := .Function.ArgumentOfType "context.Context" -}}
{{- $req := .Function.ArgumentOfType "*net/http.Request" -}}
{{- $echoCtx := .Function.ArgumentOfType "echo.Context" -}} // added
{{- if (eq $ctx "") -}}
  {{- $ctx = "ctx" -}}
  ctx := {{- if $req -}}
    {{ $req }}.Context()
  {{- else -}}
    {{- if $echoCtx -}}
      {{ $echoCtx }}.Request().Context()
    {{- else -}}
      context.TODO()
    {{- end -}}
  {{- end -}}
{{ end -}}

{{ $functionName := .Function.Name -}}
{{- $opName := $functionName -}}
{{- range .DirectiveArgs "dd:span" -}}
  {{- if eq $opName "" -}}
    {{ $opName = .Value }}
  {{- end -}}
  {{- if eq .Key "span.name" -}}
    {{- $opName = .Value -}}
    {{- break -}}
  {{- end -}}
{{- end -}}

var span tracer.Span
span, {{ $ctx }} = tracer.StartSpanFromContext({{ $ctx }}, {{ printf "%q" $opName }},
  {{- with $functionName }}
    tracer.Tag("function-name", {{ printf "%q" $functionName }}),
  {{ end -}}
  {{- range .DirectiveArgs "dd:span" }}
    {{ if eq .Key "span.name" -}}{{- continue -}}{{- end -}}
    tracer.Tag({{ printf "%q" .Key }}, {{ printf "%q" .Value }}),
  {{- end }}
)
{{- with $req }}
  {{ $req }} = {{ $req }}.WithContext({{ $ctx }})
{{- end }}
{{- with $echoCtx }} // added
  {{ $echoCtx }}.SetRequest({{ $echoCtx }}.Request().WithContext({{ $ctx }}))
{{- end }}

{{ with .Function.ResultOfType "error" -}}
  defer func(){
    span.Finish(tracer.WithError({{ . }}))
  }()
{{ else -}}
  defer span.Finish()
{{- end -}}

[RomainMuller]

No no -- it's supported any function since v0.8.0. When there is no context.Context value accessible, the span context is made available via goroutine local storage (GLS).

The aspect for //dd:span has custom code to actually wire through a context.Context value that is available for purity's sake; but also to make sure the correct context is evaluated in work that fans out to multiple goroutines (and the child work needs relating to the parent goroutine).

This might be the case for echo.Context; in which case that is definitely a feature that could be added... In which case the PR ought to be directed to the dd-trace-go hosted version of the aspect (https://github.com/DataDog/dd-trace-go/blob/a153c0d9102f3ad1d1987adb3229bd2d93e4260f/ddtrace/tracer/orchestrion.yml#L69-L123)

[korECM]

@RomainMuller
If I understand correctly, //dd:span can be used for every function, and it could apply to something like echo.Context as well—but currently, it's not applied to echo.Context, right?

And based on what you mentioned, should I modify the dd-trace-go hosted version of the aspect (DataDog/dd-trace-go@a153c0d/ddtrace/tracer/orchestrion.yml#L69-L123)?