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

make it possible to associate a ConnectionTracer with a Session #3146

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

marten-seemann
Copy link
Member

Fixes #3145.

This PR implements a combination of the options discussed on the issue:

  • logging.Tracer.NewConnectionTracer now takes a context.Context, which has values associated with it.
  • Both this context as well as the context returned from Session.Context have a context with a SessionTracingKey value (which is a uint64).
  • For outgoing connections, the context passed to NewConnectionTracer is derived from the context passed to Dial. This allows us (for example) to associate the peer ID we were dialing even if the handshake fails (and no Session is returned).

@mvdan, how does this look to you?

This context has the same value attached to it as the context returned
by Session.Context().
In the case of a dialed connection, this context is derived from the
context used for dialing.
@codecov
Copy link

codecov bot commented Apr 14, 2021

Codecov Report

Merging #3146 (878e0b2) into master (3138a45) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3146      +/-   ##
==========================================
+ Coverage   85.38%   85.41%   +0.03%     
==========================================
  Files         131      131              
  Lines        9684     9698      +14     
==========================================
+ Hits         8268     8283      +15     
+ Misses       1046     1045       -1     
  Partials      370      370              
Impacted Files Coverage Δ
qlog/qlog.go 90.60% <ø> (ø)
client.go 79.37% <100.00%> (+1.03%) ⬆️
logging/multiplex.go 97.56% <100.00%> (ø)
server.go 80.40% <100.00%> (+0.34%) ⬆️
session.go 77.24% <100.00%> (+0.04%) ⬆️
internal/utils/rand.go 75.00% <0.00%> (+12.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3138a45...878e0b2. Read the comment docs.

@mvdan
Copy link

mvdan commented Apr 16, 2021

SGTM. You could always refine this if users find it's not enough.

@marten-seemann marten-seemann merged commit 8f20817 into master Apr 19, 2021
@marten-seemann marten-seemann deleted the tracing-context branch April 19, 2021 08:14
@aschmahmann aschmahmann mentioned this pull request May 14, 2021
71 tasks
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.

find an idiomatic way to associate a tracer with a connection
2 participants