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

tracing: tighten the API to import remote traces #81020

Merged
merged 1 commit into from
May 15, 2022

Conversation

adityamaru
Copy link
Contributor

This change is a cosmetic one. It changes what was previously
called ImportRemoteSpans to ImportRemoteRecording. This is
because, the former suggests that the method can be used to
import spans from disjointed Recordings but this is not the case. We
subsume the remote spans into the receiving span as its children,
and while doing so we assume that they all belong to the same recording
with the root as the first element of the imported slice.

This change will make some of the work being done for #80391 easier
to reason about.

Release note: None

@adityamaru adityamaru requested review from andreimatei and a team May 5, 2022 02:18
@adityamaru adityamaru requested review from a team as code owners May 5, 2022 02:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru removed the request for review from a team May 5, 2022 02:19
Copy link
Collaborator

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

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

👍 The symmetry between FinishAndGetRecording and ImportRemoteRecording seems nice.

@adityamaru
Copy link
Contributor Author

TFTR!

bors r=stevendanna

@craig
Copy link
Contributor

craig bot commented May 6, 2022

Build failed:

@adityamaru
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request May 7, 2022
81020: tracing: tighten the API to import remote traces r=adityamaru a=adityamaru

This change is a cosmetic one. It changes what was previously
called `ImportRemoteSpans` to `ImportRemoteRecording`. This is
because, the former suggests that the method can be used to
import spans from disjointed Recordings but this is not the case. We
subsume the remote spans into the receiving span as its children,
and while doing so we assume that they all belong to the same recording
with the root as the first element of the imported slice.

This change will make some of the work being done for #80391 easier
to reason about.

Release note: None

Co-authored-by: Aditya Maru <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 7, 2022

Build failed:

@adityamaru
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 12, 2022

Build failed:

This change is a cosmetic one. It changes what was previously
called `ImportRemoteSpans` to `ImportRemoteRecording`. This is
because, the former suggests that the method can be used to
import spans from disjointed Recordings but this is not the case. We
subsume the remote spans into the receiving span as its children,
and while doing so we assume that they all belong to the same recording
with the root as the first element of the imported slice.

This change will make some of the work being done for cockroachdb#80391 easier
to reason about.

Release note: None
@adityamaru adityamaru force-pushed the more-tracing-cleanup branch from 05c95cd to 407f220 Compare May 14, 2022 12:46
@adityamaru
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented May 14, 2022

Build failed:

@adityamaru
Copy link
Contributor Author

Flaked on #80838

@adityamaru
Copy link
Contributor Author

Let's try again

bors r+

@craig
Copy link
Contributor

craig bot commented May 15, 2022

Build succeeded:

@craig craig bot merged commit bc5f5b7 into cockroachdb:master May 15, 2022
@adityamaru adityamaru deleted the more-tracing-cleanup branch May 15, 2022 01:31
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.

3 participants