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

implement library for tracking net.Listener connections #42

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

Conversation

groob
Copy link
Contributor

@groob groob commented Jan 8, 2019

No description provided.

ntrack/listener.go Outdated Show resolved Hide resolved
ntrack/listener.go Outdated Show resolved Hide resolved
ntrack/listener.go Outdated Show resolved Hide resolved

func (tl *trackingListener) Accept() (net.Conn, error) {
conn, err := tl.Listener.Accept()
stats.RecordWithTags(context.TODO(), []tag.Mutator{tag.Upsert(tl.stats.TagSuccess, fmt.Sprintf("%v", err == nil))}, tl.stats.ListenerAccepted.M(1))
Copy link

Choose a reason for hiding this comment

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

Perhaps you might want to have two tag keys
status --> whose values can either be "OK" and "ERROR"
error --> whose value is set only if key "status" is set and its value is the full error encountered.

This will aid a lot with grouping errors, creating alerts on various errors and determination of error/success rates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. having arbitrary strings end up as tags is a little icky, but I suspect there's only a small number of them you'd possibly encounter from the stdlib.

I can see how this would be very useful, although my instinct would be to check for the actual error strings in logs/traces.

Choose a reason for hiding this comment

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

I also don't like arbitrary strings as tags (if strings have values like port number, ip address, stacks, etc the cardinality can blow up). A compromise is to add it here but not add it by default in the respective view and have some way (either code or doc) to add it explicitly if one wants it.

I would recommend pre-defining two mutator slices and select the one to be used according to the value of err instead of creating them on every call.


func (tl *trackingListener) Accept() (net.Conn, error) {
conn, err := tl.Listener.Accept()
stats.RecordWithTags(context.TODO(), []tag.Mutator{tag.Upsert(tl.stats.TagSuccess, fmt.Sprintf("%v", err == nil))}, tl.stats.ListenerAccepted.M(1))

Choose a reason for hiding this comment

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

I also don't like arbitrary strings as tags (if strings have values like port number, ip address, stacks, etc the cardinality can blow up). A compromise is to add it here but not add it by default in the respective view and have some way (either code or doc) to add it explicitly if one wants it.

I would recommend pre-defining two mutator slices and select the one to be used according to the value of err instead of creating them on every call.

stats *Stats
}

func NewInstrumentedListener(lis net.Listener) (net.Listener, *Stats) {

Choose a reason for hiding this comment

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

Instead of using context.TODO() what about passing a ctx on the contructor and hold to it? This way caller can even add their own tags and modiiy the views accordingly.

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.

5 participants