Skip to content

Conversation

@irfansharif
Copy link
Contributor

We can rid ourselves of the opentracing cruft just a bit more, and add
some type-safety while doing so. While here, we'll also add some text
explaining how tracing data gets propagated across process boundaries.

Release note: None


First commit is from #59815, ignore here.

@irfansharif irfansharif requested review from angelapwen, knz and tbg February 5, 2021 22:49
@irfansharif irfansharif requested review from a team as code owners February 5, 2021 22:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@irfansharif irfansharif removed request for a team February 5, 2021 22:50
@irfansharif irfansharif force-pushed the 210205.improve-deser-iface branch 2 times, most recently from a110224 to 220fc37 Compare February 5, 2021 23:23
@irfansharif
Copy link
Contributor Author

CI's failing due to something else:

panic: geos: error during GEOS init: geos: cannot load GEOS from dir "/go/src/github.com/cockroachdb/cockroach/lib": geos error: /lib/x86_64-linux-gnu/libm.so.6: version `GLIBC_2.29' not found (required by /go/src/github.com/cockroachdb/cockroach/lib/libgeos.so)

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Reviewed 22 of 22 files at r1, 14 of 14 files at r2, 9 of 9 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, and @knz)


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

//
// A context wrapping the newly created span is returned, along with the span
// itself. If non-nil, the caller is responsible to eventually Finish() it.

nit: for eventually finishing it


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

//
// A context wrapping the newly created span is returned, along with the span
// itself. If non-nil, the caller is responsible to eventually Finish() it.

nit: for eventually finishing it

And improve some documentation while here.

Release note: None
@irfansharif irfansharif force-pushed the 210205.improve-deser-iface branch from 220fc37 to 930b652 Compare February 8, 2021 19:04
We can rid ourselves of the opentracing cruft just a bit more, and add
some type-safety while doing so. While here, we'll also add some text
explaining how tracing data gets propagated across process boundaries.

Release note: None
@irfansharif irfansharif force-pushed the 210205.improve-deser-iface branch from 930b652 to 9687953 Compare February 8, 2021 20:05
@irfansharif
Copy link
Contributor Author

Re-ordered the commits to decouple it from #59815. TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 8, 2021

Build succeeded:

@craig craig bot merged commit 0a3dc15 into cockroachdb:master Feb 8, 2021
@irfansharif irfansharif deleted the 210205.improve-deser-iface branch February 8, 2021 21:32
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @angelapwen, @irfansharif, @knz, and @tbg)


pkg/kv/kvserver/replica_proposal.go, line 854 at r5 (raw file):

	traceCarrier := tracing.MapCarrier{
		Map: make(map[string]string),

[nit] maybe put this into a tracing.MakeMapCarrier


pkg/util/tracing/tracer.go, line 637 at r4 (raw file):

// details.
//
// The recordings from these spans will be automatically propagated to the

This deserves some more comment, given that the forked span can outlive the parent. There will be a "partial" recording in the parent, depending when it is retrieved, right?

[nit] s/these spans/the forked span

@RaduBerinde
Copy link
Member


pkg/util/tracing/tracer.go, line 637 at r4 (raw file):

Previously, RaduBerinde wrote…

This deserves some more comment, given that the forked span can outlive the parent. There will be a "partial" recording in the parent, depending when it is retrieved, right?

[nit] s/these spans/the forked span

Ah, I see there's another PR proposing changes here, never mind

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.

4 participants