add context for connect and close functions

[bpross]

This adds functions:

• ConnectCtx
• CloseCtx
• OnConnectCtx
• OnCloseCtx

that mimic the same functions without Ctx but allow passing in a context object. This can be useful for adding telemetry, etc.

I am not a huge fan of the names of things, but since I wanted to maintain backwards compatibility with the library I couldn't really figure out a better way to do this.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 77.27273% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 82.20%. Comparing base (31cdc26) to head (f06ca55).

❗ Current head f06ca55 differs from pull request most recent head 506da6d

Please upload reports for the commit 506da6d to get more accurate results.

Files	Patch %	Lines
connection.go	61.53%	3 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   82.42%   82.20%   -0.23%     
==========================================
  Files           7        7              
  Lines         717      736      +19     
==========================================
+ Hits          591      605      +14     
- Misses         93       96       +3     
- Partials       33       35       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adamdecaf]

Should we mention that the Ctx methods take priority if both are non-nil?

[alovak]

I'm thinking about the context we pass here, and it seems that we don't use it in any of the package functions/methods. The main purpose is to pass it to the user's handlers. But then, I'm like: do we actually need this?

[adamdecaf]

Do you want to wire in otel to these contexts?

https://github.com/moov-io/base/tree/master/telemetry

[alovak]

In DB driver, when we pass the context, driver can close the connection if context is Canceled or Done, but here we don't use context and that's my concern. We accept it and pass it through to handlers so they can do something with it. Is it good enough reason for this change?

[bpross]

As @adamdecaf posted the whole point of passing through these contexts into the user defined handlers allows the user to pass through any values that they have attached to the context values. In Moov's internal case we attach our telemetry to contexts, so this allows us to attach child spans in the handlers to the root spans from calling ConnectCtx or CloseCtx. This is useful during startup/shutdown, because we can follow these units of work in telemetry instead of having them dangling.

Initially I had something like this (and something similar for OnClose):

connectionOpts.OnConnect(func(c *connection.Connection) error {
    ctx, span := StartSpan(context.Background())
    defer span.End()
    
    return signOnfunc(ctx, c)
})

this gets us most of the way, but produces a "dangling" span, so we cannot associate it with app startup, or if we just tried to reconnect, etc.

I agree that we can enhance the use of the contexts that are being passed into these methods and look for cancelation, etc. However, given the context of the work that I am doing at the moment, that would fall out of scope for that.

[adamdecaf]

Sounds good.

Can you pass the app-startup span we create to these calls? That span's End() is called when NewEnvironment returns.