Enable Trace Propagation

grant commented 3 years ago

It looks like the Go SDK doesn't have any ability to propagate the traceparent HTTP header to the CE SDK client.

The SDK only receives ce-traceparent headers, which is not W3 trace header name:

https://w3c.github.io/trace-context/#traceparent-header

Some systems produce this header and we'd like the CE SDK to parse it properly.

Below is a test.

Sample

https://github.com/cloudevents/sdk-go/blob/main/samples/http/receiver-traced/main.go

package main

import (
    "context"
    "fmt"
    "github.com/cloudevents/sdk-go/v2/client"
    "log"

    cloudevents "github.com/cloudevents/sdk-go/v2"
)

func main() {
    ctx := context.Background()
    p, err := cloudevents.NewHTTP()
    if err != nil {
        log.Fatalf("failed to create protocol: %s", err.Error())
    }

    c, err := cloudevents.NewClientObserved(p, cloudevents.WithTracePropagation)
    if err != nil {
        log.Fatalf("failed to create client, %v", err)
    }

    log.Printf("will listen on :8080\n")
    log.Fatalf("failed to start receiver: %s", c.StartReceiver(ctx, receive))
}

func receive(ctx context.Context, e cloudevents.Event) {
    _, span := client.TraceSpan(ctx, e)
    defer span.End()

    fmt.Printf("%s", e)
}

Is there any way for a developer to propagate the traceparent HTTP header to clients (not the CE-traceparent header)? I'm looking to develop a CE receiver that accepts this header. A sample would help.

Ref: https://github.com/GoogleCloudPlatform/functions-framework-go/issues/42

n3wscott commented 3 years ago

See the samples for how to use the new trace config.

grant commented 3 years ago

The sample parses a ce-traceparent, not traceparent.

Example curl:

curl localhost:8080 -XPOST \
-H "Host: localhost:8080" \
-H "User-Agent: Go-http-client/1.1" \
-H "Content-Length: 0" \
-H "Ce-Id: abc-123" \
-H "Ce-Source: cloudevents.conformance.tool" \
-H "Ce-Specversion: 1.0" \
-H "traceparent: 123" \
-H "Ce-Type: foo.bar" \
-H "Accept-Encoding: gzip"

(Only a Ce-traceparent header works)

Actual

go run main.go

...

Validation: valid
Context Attributes,
  specversion: 1.0
  type: foo.bar
  source: cloudevents.conformance.tool
  id: abc-123

Expected

go run main.go

...

Validation: valid
Context Attributes,
  specversion: 1.0
  type: foo.bar
  source: cloudevents.conformance.tool
  id: abc-123
Extensions,
  traceparent: 123

CC @anniefu

n3wscott commented 3 years ago

If you look at the spec you linked, you will see that traceparent and tracestate are cloudevent attributes and the SDK will ignore protocall specific headers like traceparent as in that binding format the header should be ce-traceparent.

I think the SDK is working as the upstream cloudevents spec intends.

grant commented 3 years ago

If you look at the spec you linked, you will see that traceparent and tracestate are cloudevent attributes and the SDK will ignore protocall specific headers like traceparent as in that binding format the header should be ce-traceparent.

I think the SDK is working as the upstream cloudevents spec intends.

Thanks for the reply @n3wscott.

From my understanding, the W3 specification for tracing specifies the header to not include the ce- prefix.

https://www.w3.org/TR/trace-context/#header-name

That is what our client sends as well.

The trace example has both ce- and non ce- headers, confusingly.

Because of the w3 spec, HTTP servers that follow that spec may provide the tracing header but not with the ce- prefix to the header.

It looks like not all document extensions follow this pattern of adding ce- as well.

dataref: https://github.com/cloudevents/spec/blob/master/extensions/dataref.md

Is there any way we could have the SDK listen to the standardized w3 header name of traceparent? Is this expectation appropriate?

We do state in the CE spec that extensions should have the ce- prefix, but this doesn't match the w3 spec: https://github.com/cloudevents/spec/blob/master/http-protocol-binding.md#3131-http-header-names

n3wscott commented 3 years ago

Personally, I think the extension handler should work in the way you are asking.

The trace example has both ce- and non ce- headers, confusingly.

I agree.

I think we could change how known extensions are handled in the sdk. Let's noodle on this:

If traceparent exists and ce-traceparent does not, propagate traceparent to ce-traceparent
- I think this could be wrong because if you are attempting to trace an http invoke between two points, that integration might not want the trace to be appended to the event.
If ce-traceparent exists on outbound http messages, we auto-mirror the header to traceparent.
- I think this is likely correct and more in line with what the cloudevents extension wants.
if both ce-traceparent and traceparent exist, ce-traceparent wins and is mirrored on the outbound request in traceparrent.

Is there any way we could have the SDK listen to the standardized w3 header name of traceparent? Is this expectation appropriate?

Yeah I think so and for what its worth, I think the request parsing parts do look at standard headers from the tracing libs but I have not been the lead on these features, I would appreciate any help here on doing the right thing.

dataref might be an easier to example to implement in the sdk, but nothing has been done to formally support that extension and add helpers.

joaopgrassi commented 2 years ago

Hey @grant! A while ago, I worked on adding OTel support for the Go SDK on HTTP.

Is there any way we could have the SDK listen to the standardized w3 header name of traceparent? Is this expectation appropriate?

Yes.

The OTel Go offers auto-instrumentation for HTTP: https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/net/http/otelhttp. So, if you use their "wrapper" around the transport and the middleware handler, any incoming/outgoing HTTP call will be properly traced/propagated.

This is what I worked on in my PR. When configuring the CloudEvents Go SDK, you can create the HTTP Client that is already properly instrumented with OTel, and all will work out of the box. You can check this sample I created https://github.com/cloudevents/sdk-go/tree/main/samples/http/otel-sender-receiver that has a sender-receiver via HTTP. You can also start the Jaeger container to see the traces.

For the ce- part, I wasn't aware we have that but with the latest discussions around context propagation in OpenTelemetry, we might need to use it to attach the same span context that is created when you create an event. It's a separated topic from what you asked but just wanted to point it out. For standard W3C propagation, if you use what I mentioned above it all will work.

joaopgrassi commented 2 years ago

I should add that the sample you linked https://github.com/cloudevents/sdk-go/blob/main/samples/http/receiver-traced/main.go is using the "old" opentracing instrumentation. You might consider using OpenTelemetry which is the merge of OpenTracing and OpenCensus.

Another thing is that by using the HTTP auto-instrumentation from OpenTelemetry, you only get insights into transport spans. So you will have the HTTP GET, POST. These are "generic" and don't contain any event information. That's what the ObservabilityService in the Go SDK does - it creates an additional span containing the event info. This is based on the PR you already saw: https://github.com/open-telemetry/opentelemetry-specification/pull/1978 and will most likely change as we progress there in the spec, but you can already use it today.

grant commented 2 years ago

Thanks a bunch.

I also found this sample, which is very useful. https://github.com/cloudevents/sdk-go/blob/main/v2/extensions/distributed_tracing_extension.go

Will continue to test things on my side.

cloudevents / sdk-go