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

MYST-689. Added sending last stats on disconnect #325

Merged
merged 4 commits into from Sep 7, 2018

Conversation

Projects
None yet
3 participants
@soffokl
Copy link
Member

commented Aug 30, 2018

No description provided.

@soffokl soffokl added the enhancement label Aug 30, 2018

@soffokl soffokl self-assigned this Aug 30, 2018

@soffokl soffokl requested a review from mysteriumnetwork/core Aug 30, 2018

@soffokl soffokl requested review from tadovas and Waldz as code owners Aug 30, 2018

@soffokl soffokl force-pushed the stats-on-disconnect branch from 3f1e90f to 2a3bcae Aug 30, 2018

@tadovas
Copy link
Member

left a comment

Handling connection state related events is not seems to be the right place inside byte statistics middleware, as now it does two things instead of one -> reports bytes stats, calls stop handlers in case of stop. I think that a better approach would be to handle this thing on higher lever - inside connection manager, as that component tracks connection state and receives callbacks about disconnect events, it also has reference to stats sender. Let's discuss this in more details

@@ -49,19 +55,21 @@ func (middleware *middleware) Start(commandWriter management.CommandWriter) erro
}

func (middleware *middleware) Stop(commandWriter management.CommandWriter) error {
defer middleware.postStopHandle()

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 31, 2018

Member

I imagined this feature as separate middleware, which listens for Openvpn events and hooks on DISCONNECT event or smtg.
Do we need stats about "stoping middleware" or "connection end"?

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 31, 2018

Member

Exactly. But I and @soffokl already discussed it, and decided that he will take a different approach

@soffokl soffokl force-pushed the stats-on-disconnect branch from 1a0216e to b6a0587 Sep 4, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

@tadovas @Waldz could you take a look on this once again?

@tadovas
Copy link
Member

left a comment

Overall looks good. Just strange test, and leftover (callback function in middleware on stop)

type middleware struct {
sessionStatsHandler SessionStatsHandler
interval time.Duration
postStop []func()

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

Leftover, I guess?

middleware := NewMiddleware(statsRecorder.record, 1*time.Second)
mockConnection := &management.MockConnection{}
middleware.Stop(mockConnection)
assert.Equal(t, "bytecount 0", mockConnection.LastLine)

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

What is the test case here?

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 5, 2018

Author Member

We had a Test_Start a bit upper in the same file. It checks the correctness of the send command on the middleware Start.
I have added the similar test, but for Stop case.
It was added to keep consistency for these tests.

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

Oh, now I got it. Start /stop enables/disables bytecount notifications receiving on requested interval. Yea this looks ok now.

@@ -24,12 +24,14 @@ import (
"strings"
)

var (

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

Nice regexp cleanup 👍


client server.Client
signer identity.Signer
statsKeeper bytescount.SessionStatsKeeper

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 5, 2018

Member

Is it possible to move SessionStatsKeeper from middleware too? It would be nice improvement

providerID identity.Identity
consumerCountry string

client server.Client

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 5, 2018

Member

We call it mysteriumClient

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

@Waldz @tadovas requested changes added, please, take a look on this.

soffokl added some commits Sep 4, 2018

@soffokl soffokl force-pushed the stats-on-disconnect branch from bb756ab to 4eaacce Sep 6, 2018

@soffokl

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

@Waldz @tadovas This PR rebased to the master. Another approval required.

@@ -17,9 +17,11 @@

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 6, 2018

Member

This has to move out from middlewares too.

@@ -22,14 +22,17 @@ import (
"strconv"
"time"

"github.com/mysteriumnetwork/node/client/stats"

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 6, 2018

Member

It feels like this dependency should not be here. Some deep internal openvpn package (middleware) knows about one of possible stats consumers. Maybe as an alternative, statistics and state callbacks can be extracted as separate interfaces in top openvpn package and reused by both sides

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Member

I imagine 2 packages in future:

  • go-openvpn - the one which we opensource for anybody to use
  • Openvpn Adapter - our specific openvpn integration to Mysterium Node

So, it should belong to Openvpn Adapter. I suggest openvpn/session/stats

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 6, 2018

Member

Openvpn adapter should become openvpn service provider plugin :)

@Waldz Waldz merged commit 4eaacce into master Sep 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the stats-on-disconnect branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.