sdk-javascript

sdk-javascript copied to clipboard

Is your feature request related to a problem? Please describe. A Distributed Tracing Extension is part of the specification. Javascript SDK should support it.

Describe the solution you would like to see An implementation of Distributed Tracing should be implemented in similar way as it is done for Go and Java SDK's.

/cc @piecioshka

@cardil good catch!

A Distributed Tracing Extension is part of the specification. Javascript SDK should support it.

To be clear, expectations for the SDKs do not require that this be implemented. https://github.com/cloudevents/spec/blob/master/SDK.md#technical-requirements

But I agree - it would be good.

This issue is stale because it has been open 30 days with no activity.

@cardil I've been thinking about this, and I wonder how you see this in practical usage. In the spec, the example given is,

CURL -X POST example/webhook.json \
-H 'ce-id: 1' \
-H 'ce-specversion: 1.x-wip' \
-H 'ce-type: example' \
-H 'ce-source: http://localhost' \
-H 'ce-traceparent:  00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01' \
-H 'traceparent:  00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01' \
-H 'tracestate: rojo=00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01,congo=lZWRzIHRoNhcm5hbCBwbGVhc3VyZS4`

Receiving an event message like this in the SDK would result in a CloudEvent that had the extension traceparent. For example assuming those headers above,

const event = HTTP.toEvent(headers, body);
console.log(event.traceparent); // 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01

Producing an event, that includes this information is similarly straightforward.

const event = new CloudEvent({ source: '/example', type: 'tracer', traceparent: '00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01' });
const message = HTTP.binary(event);
console.log(message.headers); // should include a 'ce-traceparent: 00-0af7651916cd43dd8448eb211c80319c-b9c7c989f97918e1-01' header

Looking at the spec, I'm not sure there is anything else for us to support. Is there some specific behavior that you had in mind?

If what we have already is sufficient to consider the Distributed Tracing extension supported, we should add this to the README.md and maybe provide an example.

Awesome. I think we can easily resolve this issue with just writing those examples above in documentation.

I assume tracestate is also supported, just as traceparent?

I think if both of those are supported, we should write those in documentation as examples, and maybe also add a unit test to make sure this behavior will be kept.

I assume tracestate is also supported, just as traceparent?

Yes - it behaves just as any extension. A ce-<extname>: <extvalue> header will get created when creating new events, and a ce-<extname> header will result in that <extname> being a property of the event.

@lance Looking at Golang implementation it seems like more than just passing those extensions should be done:

https://github.com/cloudevents/sdk-go/blob/8ff3fbc/v2/client/client_observed.go#L48-L57

@cardil I have started a thread in the CNCF Slack to discuss what exactly is expected for the distributed tracing extension in terms of SDK support.

https://cloud-native.slack.com/archives/CCXF3CBL1/p1606776741094000

@cardil the golang implementation was done before the latest changes to the specification. The goal of the extension is not to carry the actual tracing informations (aka the actual traceparent), but the historic one. For more details, look here: https://github.com/cloudevents/spec/blob/master/extensions/distributed-tracing.md#using-the-distributed-tracing-extension

If you need to carry the tracing informations, you must not use the distributed tracing extension but you should use the usual http specs for that (eg the w3c trace context spec)

So is the actual action item here just adding docs? That is sort of what i got from skimming this thread

So is the actual action item here just adding docs? That is sort of what i got from skimming this thread

I think so, yes

Docs with code examples

This issue is stale because it has been open 30 days with no activity.

This issue is stale because it has been open 30 days with no activity.

@lance I think this issue is complete? Anything else this issue is waiting on?

@lance I think this issue is complete? Anything else this issue is waiting on?

Well, I think it would probably be good to document how a user adds traces to an event. Do you think we should just close this issue and open another one? Maybe just changing the title to something like "Document support for distributed tracing". :shrug:

In terms of how a user uses this:

A user should be able to add the HTTP header traceparent: 01-asdfasdf-etc and see the value within their CloudEvent receiver, such as a property cloudevent.extensions.traceparent.

A user should be able to add the HTTP header traceparent: 01-asdfasdf-etc and see the value within their CloudEvent receiver, such as a property cloudevent.extensions.traceparent.

They'd add it as ce-traceparent. Here is an example. https://github.com/cloudevents/spec/blob/v1.0.1/extensions/distributed-tracing.md#using-the-distributed-tracing-extension

I believe this would just show up as an extension attribute naturally. We already have a test for a random extension here, which is essentially the same thing, just not using traceparent. I suppose we could add an explicit test.

https://github.com/cloudevents/sdk-javascript/blob/f7b2840f826be8294f5357ab3b9a530170148dfc/test/integration/message_test.ts#L299-L300

This issue is stale because it has been open 30 days with no activity.