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

Stats reporting #33

Open
wants to merge 3 commits into
base: conjure
Choose a base branch
from
Open

Stats reporting #33

wants to merge 3 commits into from

Conversation

nzimm
Copy link

@nzimm nzimm commented Nov 6, 2019

Report stats protobuf to station, over the socks5 address provided by Psiphon

@nzimm nzimm requested a review from jmwample November 6, 2019 17:32
@ewust
Copy link
Member

ewust commented Nov 6, 2019

Does this also set the stats protobuf somewhere, or is there code that does that already?

It would be good to have corresponding code on the server / a plan for how we get these stats into gobbler/our infrastructure. Where do we plan to ultimately host the server for this? ec2? on the detector?

@jmwample
Copy link
Member

jmwample commented Nov 6, 2019

I was thinking host on ec2. it doesn't need to be colocated with the station

The stats info is set throughout the registration and connection process, but we can add things to the protobuf that we want to track. One thing I haven't added that would be good is failed decoys.

@@ -35,3 +40,36 @@ func Logger() *logrus.Logger {
})
return logrusLogger
}

func StatsReporting(stats *pb.SessionStats) {
Copy link
Member

Choose a reason for hiding this comment

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

There are lots of TODOs and commented out lines of code in this function.
Do we need all of them, or can we remove some?


conn, err := registration.Connect(ctx)
if conn != nil && err == nil {
StatsReporting(cjSession.stats)
Copy link
Member

Choose a reason for hiding this comment

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

Make this a go routine as this will block the return of con to the user by a full tcp (tls) session

@nzimm
Copy link
Author

nzimm commented Nov 7, 2019

I've cleaned up commented code and TODOs, besides the one with regards to a stats endpoint for the protobuf reporting


conn, err := registration.Connect(ctx)
if conn != nil && err == nil {
go StatsReporting(cjSession.stats)
Copy link
Member

Choose a reason for hiding this comment

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

This is on me, but both cjSession, and registration track stats and I'm not sure exactly which of the stats we want to send via this channel. I think we probably want registration.stats. This is mostly an issue of deciding exactly what we want to track and send over this channel.

It may be worth opening a different issue to deal with this, as this change does complete the task of setting up the channel to communicate stats which is the issue that we opened here.

thoughts @nzimm?

Copy link
Author

Choose a reason for hiding this comment

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

I think reporting the correct stats protobuf probably falls under the scope of this ticket.
I'm just not sure what is tracked differently between cjSession.stats and registration.stats

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.

4 participants