Skip to content

tracing: move off opentracing interfaces#56012

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
tbg:tracing-cleanup-internal
Oct 28, 2020
Merged

tracing: move off opentracing interfaces#56012
craig[bot] merged 2 commits intocockroachdb:masterfrom
tbg:tracing-cleanup-internal

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Oct 27, 2020

We were previously shoehorning our tracing subsystem into the
opentracing framework. This resulted in lots of bending over
backwards to no real gain.

Move off the interfaces so that we can more easily refactor
the code for always-on tracing as well as make it more digestible.

This commit is mostly mechanical. A bunch of code around the
RPC interceptors was cribbed from the otgrpc package.

Cleanups are delegated to additional commits.

TestFollowerReadsWithStaleDescriptor, skipped temporarily in #55937,
passes again due to the rearrangement in startSpanGeneric (we
were setting the spanID too late, which led to tags being dropped).

Release note: None

@tbg
tracing: rename a file
c7c2798 
`span.go` is where you'd go look for `type Span`.

Release note: None
@tbg tbg requested a review from a team as a code owner October 27, 2020 09:08
@tbg tbg added A-kv-observability A-tracing Relating to tracing in CockroachDB. labels Oct 27, 2020
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Oct 27, 2020

This change is Reviewable

@tbg tbg requested review from irfansharif and knz and removed request for a team October 27, 2020 09:08
@tbg tbg force-pushed the tracing-cleanup-internal branch 3 times, most recently from 0b7068c to 4e9be36 Compare October 27, 2020 10:55
@tbg
Copy link
Copy Markdown
Member Author

tbg commented Oct 27, 2020

@andreimatei no need to review, but I sense that you will like this

knz
knz approved these changes Oct 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: that's clear thanks

Reviewed 3 of 3 files at r1, 66 of 66 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif)

irfansharif
irfansharif approved these changes Oct 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

#56004 should make things clearer in CI but reminder to run bazel run //:gazelle (or make bazel-generate) after the each commit (rename + new file). We should probably add git commit hooks or something here to do this automatically. Sorry!

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @tbg)

pkg/util/tracing/tracer.go, line 657 at r2 (raw file):

type activeSpanKey struct{}

func SpanFromContext(ctx context.Context) *Span {

I think we'll want comments here over this exported method (and below) to avoid linter failures.

@tbg tbg force-pushed the tracing-cleanup-internal branch from 4e9be36 to bb6e9a6 Compare October 27, 2020 15:42
@tbg
Copy link
Copy Markdown
Member Author

tbg commented Oct 27, 2020

Nice to see those and happily obliging:

  lint_test.go:1504: 
            pkg/util/tracing/span.go:970:16: func (*Span).BaggageItem is unused (U1001)
        lint_test.go:1504: 
            pkg/util/tracing/span.go:980:20: func (*crdbSpan).BaggageItem is unused (U1001)
        lint_test.go:1504: 
            pkg/util/tracing/span.go:992:16: func (*Span).LogEvent is unused (U1001)
        lint_test.go:1504: 
            pkg/util/tracing/span.go:1000:16: func (*Span).LogEventWithPayload is unused (U1001)
        lint_test.go:1504: 
            pkg/util/tracing/span.go:1008:16: func (*Span).Log is unused (U1001)

@tbg
tracing: avoid opentracing interfaces internally
429ab2d 
We were previously shoehorning our tracing subsystem into the
opentracing framework. This resulted in lots of bending over
backwards to no real gain.

Move off the interfaces so that we can more easily refactor
the code for always-on tracing as well as make it more digestible.

This commit is mostly mechanical. A bunch of code around the
RPC interceptors was cribbed from the `otgrpc` package.

Cleanups are delegated to additional commits.

TestFollowerReadsWithStaleDescriptor, skipped temporarily in cockroachdb#55937,
passes again due to the rearrangement in `startSpanGeneric` (we
were setting the spanID too late, which led to tags being dropped).

Release note: None
@tbg tbg force-pushed the tracing-cleanup-internal branch from bb6e9a6 to 429ab2d Compare October 27, 2020 17:00
@tbg
Copy link
Copy Markdown
Member Author

tbg commented Oct 28, 2020

bors r=knz,irfansharif

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 28, 2020

Build succeeded:

@craig craig bot merged commit 7442f71 into cockroachdb:master Oct 28, 2020
@tbg tbg deleted the tracing-cleanup-internal branch October 28, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@knz knz knz approved these changes

@irfansharif irfansharif irfansharif approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-kv-observability A-tracing Relating to tracing in CockroachDB.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tbg @cockroach-teamcity @knz @irfansharif