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

feat: add basic tracing #769

Merged
merged 2 commits into from
Mar 30, 2023
Merged

feat: add basic tracing #769

merged 2 commits into from
Mar 30, 2023

Conversation

guseggert
Copy link
Contributor

No description provided.

@guseggert guseggert requested a review from iand April 29, 2022 00:55
@guseggert guseggert marked this pull request as ready for review April 29, 2022 00:55
dht.go Outdated Show resolved Hide resolved
@BigLep BigLep added this to the Best Effort Track milestone Jun 3, 2022
@Jorropo
Copy link
Contributor

Jorropo commented Mar 29, 2023

This generate outputs like this:

Capture d’écran du 2023-03-29 05-31-12

dht.go Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
routing.go Outdated Show resolved Hide resolved
routing.go Outdated Show resolved Hide resolved
routing.go Outdated Show resolved Hide resolved
rtrefresh/rt_refresh_manager.go Outdated Show resolved Hide resolved
rtrefresh/rt_refresh_manager.go Show resolved Hide resolved
rtrefresh/rt_refresh_manager.go Outdated Show resolved Hide resolved
rtrefresh/rt_refresh_manager.go Outdated Show resolved Hide resolved
rtrefresh/rt_refresh_manager.go Outdated Show resolved Hide resolved
@guillaumemichel
Copy link
Contributor

Any reasons not to add a context as argument to Refresh?

// RefreshRoutingTable requests the refresh manager to refresh the Routing Table.
// If the force parameter is set to true true, all buckets will be refreshed irrespective of when they were last refreshed.
//
// The returned channel will block until the refresh finishes, then yield the
// error and close. The channel is buffered and safe to ignore.
func (r *RtRefreshManager) Refresh(force bool) <-chan error {
resp := make(chan error, 1)
r.refcount.Add(1)
go func() {
defer r.refcount.Done()
select {
case r.triggerRefresh <- &triggerRefreshReq{respCh: resp, forceCplRefresh: force}:
case <-r.ctx.Done():
resp <- r.ctx.Err()
}
}()
return resp
}

@Jorropo
Copy link
Contributor

Jorropo commented Mar 29, 2023

Any reasons not to add a context as argument to Refresh ?

@guillaumemichel thx for the review <3

Refresh merge concurrent calls into one single refresh (if a refresh is in flight and you try to refresh once more, nothing happens and you return once the already in flight refresh finishes).

If we start accepting contextes we will have to do a dance of keeping a tally of how many calls still want the refresh, because a cancel of one of them shouldn't cancel the full refresh if other peoples are relying on it.

I could add a context here but it would make the code more complex for what seems a small benefit over using r.ctx.

@Jorropo Jorropo force-pushed the feat/otel-tracing branch 2 times, most recently from 9605ac9 to ef4c73a Compare March 29, 2023 21:56
Removed noisy traces and added tracing to RtRefreshManager
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

LGTM! I am OK with merging, we can always add more spans later if needed.

@Jorropo Jorropo merged commit dd27ddd into master Mar 30, 2023
@Jorropo Jorropo deleted the feat/otel-tracing branch March 30, 2023 07:01
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.

5 participants