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

We patch OpenCensus to add SetSpanID method #2052

Closed
grantr opened this issue Oct 17, 2019 · 5 comments · Fixed by #2873
Closed

We patch OpenCensus to add SetSpanID method #2052

grantr opened this issue Oct 17, 2019 · 5 comments · Fixed by #2873
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@grantr
Copy link
Contributor

grantr commented Oct 17, 2019

#1972 added a patch to the vendored opencensus library that adds the SetSpanID method.
@Harwayne calls this a viral patch: any project importing eventing will also need to patch their opencensus.

This was used only as a last resort because OpenCensus doesn't provide the functionality we need to set the parent span id (see pkg/tracing/traceparent.go). We don't expect OpenCensus to fix this issue, but eventually we'll switch to OpenTelemetry which will hopefully make the patch unnecessary.

Everywhere this patch is used, we should add a comment that links to this bug.

Expected behavior
We never patch vendored code.

To Reproduce

  1. Try importing knative.dev/eventing/pkg/tracing, or knative.dev/eventing and one of the eventing commands that supports tracing.
  2. Observe the build fails with vendor/knative.dev/eventing/pkg/tracing/traceparent.go:83:6: span.SetSpanID undefined (type *trace.Span has no field or method SetSpanID)

Knative release version
0.10.0

@grantr grantr added the kind/bug Categorizes issue or PR as related to a bug. label Oct 17, 2019
@grantr grantr changed the title We patch opencensus to add SetSpanID method We patch OpenCensus to add SetSpanID method Oct 17, 2019
k15r added a commit to k15r/kyma that referenced this issue Nov 6, 2019
We know this is ugly, but there is currently no other way
see knative/eventing#2052
@mattmoor
Copy link
Member

We don't expect OpenCensus to fix this issue

Have we asked them? Have we opened a PR to add this functionality upstream?

This was a bit of a shock for me because generally when we do this we reference the version we're backporting it from (e.g. higher kube version) or the PR where we're adding it upstream, but there are no discernable attempts to do so here.

@grantr
Copy link
Contributor Author

grantr commented Jan 28, 2020

I believe there was certainty that this change would not be allowed, but I don't remember the details and haven't been able to find them after some digging. It might be worth contacting OpenCensus again to get some fresh eyes.

@grantr
Copy link
Contributor Author

grantr commented Feb 7, 2020

I asked around and it seems this change was never floated to the OpenCensus community. So

Have we asked them? Have we opened a PR to add this functionality upstream?

No, and no, but should and should :)

@mattmoor
Copy link
Member

mattmoor commented Feb 8, 2020

@grantr thanks and thanks :)

@ian-mi
Copy link
Contributor

ian-mi commented Feb 9, 2020

It appears that this patch is needed to adopt the parent span without creating a new child span. Have we considered whether the correct behavior is actually to create a new child span in the channel dispatcher? I can see this span being potentially useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants