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

Remove distributed tracing extension #751

Conversation

slinkydeveloper
Copy link
Member

@slinkydeveloper slinkydeveloper commented Dec 17, 2020

This PR removes the distributed tracing extension spec. Below just some answers to the questions you're probably gonna ask to me:

Why?

The reason to remove the distributed tracing extension is very simple: from the user perspective, this spec that leads users to think to it as a replacement for w3c trace context.

Why people are confused?

We receive a lot of times questions, both in spec community channels and in sdks, about how to use the distributed tracing as medium to transport trace informations. And it's often hard to get people to understand that the goal of the extension is to carry the historic traceparent, and MUST not be used as a replacement for w3c trace context and friends.

I believe that if a spec contains an unclear feature like that, then it should be removed, because such feature leads to bad implementations which doesn't interact properly between themselves. Also, I think it's important to be clear that we're not trying to overlap with other tracing specs already widely adopted outside CloudEvents.

But we just changed recently, right?

Yes, we recently approved some changes to make more clear its use case #607. This still doesn't help and lead people to confusion, probably because of the name and the extension keys that still resembles the w3c trace context.

I don't want to remove this, because I'm using this extension as replacement to w3c trace context.

You're using it wrong, change your code.

I don't want to remove this, because I'm using this extension to carry historic trace informations.

Just continue to use it, removing an extension from this repository doesn't mean that nobody can use it anymore.
If we really feel that this extension spec should be "sponsored" by the Serverless WG because we have a lot of "correct" use cases for that, then I propose to rename both the spec name and the extension fields to a name that clearly disambiguates from w3c trace context. For example, something like that might work:

  • Distributed Tracing Extension -> Historic Trace Extension
  • traceparent -> initialtraceparent
  • tracestate -> initialtracestate

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper changed the title Removed distributed tracing extension Remove distributed tracing extension Dec 17, 2020
@duglin
Copy link
Collaborator

duglin commented Jan 14, 2021

On the 1/14 call @clemensv agreed to take a stab at adding some clarifying text

@slinkydeveloper
Copy link
Member Author

Recent discussions about Distributed Tracing Extension "unclear role":

@duglin
Copy link
Collaborator

duglin commented Jan 26, 2021

@clemensv I believe this one is waiting on some suggested text from you, right?

@jskeet
Copy link
Contributor

jskeet commented Apr 23, 2021

Do we have any progress on this? I'm mostly asking because I'm not sure whether to remove the representation in the C# SDK before we go to V2.

@duglin
Copy link
Collaborator

duglin commented Apr 24, 2021

Poke your partner in crime 🤣. @clemensv

@jskeet
Copy link
Contributor

jskeet commented Jun 3, 2021

@clemensv: Any updates on this? Getting close to a V2 release of the C# SDK...

@slinkydeveloper
Copy link
Member Author

@jskeet from a SDK mantainer POV, maybe it's a better bet to remove it for the v2 release and eventually re-add it later?

@jskeet
Copy link
Contributor

jskeet commented Jun 3, 2021

@slinkydeveloper: That's my plan unless we get anything definitive soon, yes.

jskeet added a commit to jskeet/sdk-csharp that referenced this pull request Jun 3, 2021
The future of these is currently in doubt - see cloudevents/spec#751

(We can revert this commit later if they come back.)

Fixes cloudevents#125

Signed-off-by: Jon Skeet <jonskeet@google.com>
jskeet added a commit to jskeet/sdk-csharp that referenced this pull request Jun 3, 2021
The future of these is currently in doubt - see cloudevents/spec#751

(We can revert this commit later if they come back.)

Fixes cloudevents#125

Signed-off-by: Jon Skeet <jonskeet@google.com>
jskeet added a commit to cloudevents/sdk-csharp that referenced this pull request Jun 3, 2021
The future of these is currently in doubt - see cloudevents/spec#751

(We can revert this commit later if they come back.)

Fixes #125

Signed-off-by: Jon Skeet <jonskeet@google.com>
jskeet added a commit to jskeet/sdk-csharp that referenced this pull request Jun 7, 2021
This is the first release candidate for version 2.0.0.

Most significant changes since 2.0.0-beta.3:

- Web hook code removed for the moment
  - See cloudevents/spec#781 for background
- CloudEventJsonInputFormatter removed from ASP.NET Core package
  - More work is required before we want to commit to this; code is in the sample for now
- Distributed tracing extension attributes removed for the moment
  - See cloudevents/spec#751 for background
- Use `ReadOnlyMemory<byte>` instead of `byte[]` in `CloudEventFormatter`

The last of these is the biggest change, and a breaking one.
`BinaryDataUtilities` has been modified to provide helper methods.

Signed-off-by: Jon Skeet <jonskeet@google.com>
jskeet added a commit to cloudevents/sdk-csharp that referenced this pull request Jun 7, 2021
This is the first release candidate for version 2.0.0.

Most significant changes since 2.0.0-beta.3:

- Web hook code removed for the moment
  - See cloudevents/spec#781 for background
- CloudEventJsonInputFormatter removed from ASP.NET Core package
  - More work is required before we want to commit to this; code is in the sample for now
- Distributed tracing extension attributes removed for the moment
  - See cloudevents/spec#751 for background
- Use `ReadOnlyMemory<byte>` instead of `byte[]` in `CloudEventFormatter`

The last of these is the biggest change, and a breaking one.
`BinaryDataUtilities` has been modified to provide helper methods.

Signed-off-by: Jon Skeet <jonskeet@google.com>
@slinkydeveloper
Copy link
Member Author

Can we go ahead and merge this?

@slinkydeveloper
Copy link
Member Author

@duglin

@duglin
Copy link
Collaborator

duglin commented Jul 5, 2021

@clemensv any update on this?

@grant
Copy link
Member

grant commented Nov 4, 2021

(Viewing CNCF SWG meeting minutes)

Are we removing the documented CloudEvent extension, supporting traceparent and tracestate CloudEvent attributes? With events receivers, I can imagine it would be useful to consume a trace, store it, and/or input the trace into a different API for telemetry (Cloud Trace in Google's case).

Currently, different SDKs handle this attribute differently (some accepting CE-traceparent, others traceparent, making it difficult to integrate on our side. The spec example seems to have an example of both headers. (We'd like to send apps JSON CEs with a traceparent/tracestate attribute).

Either way, clarity on the spec and if this PR is going through would help.

CC @matthewrobertson @anniefu

@clemensv
Copy link
Contributor

clemensv commented Nov 4, 2021

We are closing this issue without action.

This extension will be kept as the OpenTelemetry WG for Messaging Sementics is interested in using and potentially updating it to aligned with the outcome of their work. Chair of that effort is @pyohannes.

The gist of the state of discussion in OTel is that there is a need for end-to-end tracing at the level of CloudEvents as well as for tracing at the level of the transport mechanisms, and while those may root in the same base activity, the resulting trace paths and the motivations are different.

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.

None yet

5 participants