moov-io / iso8583-connection

:satellite: Go-powered ISO8583 connection handler offering advanced binary framing, message interleaving, and a robust connection pool for load distribution and seamless reconnections.
Apache License 2.0
70 stars 23 forks source link

add context for connect and close functions #77

Closed bpross closed 6 months ago

bpross commented 6 months ago

This adds functions:

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 commented 6 months ago

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).

:exclamation: 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 :warning:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

adamdecaf commented 6 months ago

Do you want to wire in otel to these contexts?

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

alovak commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Sounds good.

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