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

Added UDPOPtionSetters: ClientNet and ClientAddr. Setters bug fixed. … #42

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ _testmain.go
*.exe
*.test
*.prof

# IDE
.idea/
3 changes: 2 additions & 1 deletion log.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ func (j *JSONFormatter) Format(e *log.Entry) ([]byte, error) {
return (&j.JSONFormatter).Format(e)
}

var r = regexp.MustCompile(`github\.com/(S|s)irupsen/logrus`)
var r = regexp.MustCompile(`github\.com/(S|s)irupsen/logrus|\<autogenerated\>`)


func findFrame() int {
for i := 3; i < 10; i++ {
Expand Down
28 changes: 25 additions & 3 deletions udphook.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,30 @@ const (
// UDPOptionSetter represents functional arguments passed to ELKHook
type UDPOptionSetter func(f *UDPHook)

// ClientNet sets clientNet option in UDPHook
// Usage: hook, err := NewUDPHook(ClientNet("tcp"))
func ClientNet(clientNet string) UDPOptionSetter {
return func(f *UDPHook) {
f.clientNet = clientNet
}
}
// ClientAddr sets clientAddr option in UDPHook
// Usage: hook, err := NewUDPHook(ClientAddr("192.168.1.1:65000"))
func ClientAddr(clientAddr string) UDPOptionSetter {
Copy link
Contributor

Choose a reason for hiding this comment

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

overall makes sense, one request though - instead of two functional argument parameters, let's make it one:

func Target(network, addr string) UDPOptionSetter {
    return func(f *UDPHook) {
          f.targetNet = network
          f.targetAddr = addr
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that makes sense, simplifying calls. I can add it. But I think what if somebody needs to set only one option? Or later we need to add 3rd option? I guess setting options one-by-one forces more writing, but makes things more flexible. One can set options only he/she needs and in any order. What do you think of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

we can address those problems later, I don't think this is a big deal for now

Copy link
Author

Choose a reason for hiding this comment

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

I've added Target to set them at one call.

return func(f *UDPHook) {
f.clientAddr = clientAddr
}
}

// Target sets clientNet and clientAddr option in UDPHook in one call
// Usage: hook, err := NewUDPHook(Target("udp", "192.168.1.1:65000"))
func Target(network, addr string) UDPOptionSetter {
return func(f *UDPHook) {
f.clientNet = network
f.clientAddr = addr
}
}

// NewUDPHook returns logrus-compatible hook that sends data to UDP socket
func NewUDPHook(opts ...UDPOptionSetter) (*UDPHook, error) {
f := &UDPHook{}
Expand Down Expand Up @@ -57,7 +81,6 @@ type UDPHook struct {

type Frame struct {
Time time.Time `json:"time"`
Type string `json:"type"`
Copy link
Author

Choose a reason for hiding this comment

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

Field Type removed as it a reserved word and causes conflicts in Elastic

Entry map[string]interface{} `json:"entry"`
Message string `json:"message"`
Level string `json:"level"`
Expand All @@ -74,7 +97,6 @@ func (elk *UDPHook) Fire(e *log.Entry) error {
}
data, err := json.Marshal(Frame{
Time: elk.Clock.Now().UTC(),
Type: "trace",
Copy link
Author

@lisitsky lisitsky Dec 19, 2017

Choose a reason for hiding this comment

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

Field Type removed as it a reserved word and causes conflicts in Elastic

Entry: entry.Data,
Message: entry.Message,
Level: entry.Level.String(),
Expand All @@ -89,7 +111,7 @@ func (elk *UDPHook) Fire(e *log.Entry) error {
}
defer conn.Close()

resolvedAddr, err := net.ResolveUDPAddr("udp", "127.0.0.1:5000")
resolvedAddr, err := net.ResolveUDPAddr(elk.clientNet, elk.clientAddr)
if err != nil {
return Wrap(err)
}
Expand Down
125 changes: 125 additions & 0 deletions udphook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ package trace

import (
"io/ioutil"
"net"
"sync"
"testing"
"time"

"github.com/jonboulle/clockwork"
log "github.com/sirupsen/logrus"
Expand All @@ -44,3 +47,125 @@ func (s *HooksSuite) TestSafeForConcurrentAccess(c *C) {
}(entry)
}
}

func TestClientNet(t *testing.T) {
tests := []struct {
name string
clientNet string
want string
}{
{"Empty value", "", ""},
{"UDP", "udp", "udp"},
{"TCP", "tcp", "tcp"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := ClientNet(tt.clientNet)
hook := UDPHook{}
f(&hook)

if hook.clientNet != tt.want {
t.Errorf("ClientNet() = %v, want %v", hook.clientNet, tt.want)
}
})
}
}

func TestClientAddr(t *testing.T) {
tests := []struct {
name string
clientAddr string
want string
}{
{"Empty value", "", ""},
{"Localhost and another port", "127.0.0.1:9999", "127.0.0.1:9999"},
{"Another host", "192.168.0.1:9999", "192.168.0.1:9999"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := ClientAddr(tt.clientAddr)
hook := UDPHook{}
f(&hook)

if hook.clientAddr != tt.want {
t.Errorf("ClientAddr() = %v, want %v", hook.clientAddr, tt.want)
}
})
}
}

func TestTarget(t *testing.T) {
type args struct {
network string
addr string
}
tests := []struct {
name string
args args
wantNet string
wantAddr string
}{
{"Empty value", args{"", ""}, "", ""},
{"Localhost and another port", args{"udp", "127.0.0.1:9999"}, "udp", "127.0.0.1:9999", },
{"Another host", args{"tcp", "192.168.0.1:9999"}, "tcp","192.168.0.1:9999"},

}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := Target(tt.args.network, tt.args.addr)
hook := UDPHook{}
f(&hook)

if hook.clientAddr != tt.wantAddr {
t.Errorf("Target() addr = %v, want %v", hook.clientAddr, tt.wantAddr)
}
if hook.clientNet != tt.wantNet {
t.Errorf("ClientAddr() = %v, want %v", hook.clientNet, tt.wantNet)
}
})
}
}


func TestUDPHook_Fire(t *testing.T) {
remoteAddr := "127.0.0.1:5501"
raddr, err := net.ResolveUDPAddr("udp", remoteAddr)
UDPConn, err := net.ListenUDP("udp", raddr)
if err != nil {
t.Errorf("Cannot listen given address %v, error: %v", remoteAddr, err)
return
}
defer UDPConn.Close()

var ok bool

var (
wg sync.WaitGroup
bytesRead int
serverReadError error
)
wg.Add(1)
go func() {
defer wg.Done()
var b = make([]byte, 1024)
UDPConn.SetDeadline(time.Now().Add(100 * time.Millisecond))
bytesRead, _, serverReadError = UDPConn.ReadFromUDP(b)
if bytesRead > 0 {
ok = true
}
serverReadError = err
}()

hook, err := NewUDPHook(ClientAddr(remoteAddr + ""))
logger := log.New()
entry := log.NewEntry(logger)

err = hook.Fire(entry)
if err != nil {
t.Errorf("Fire() error: %v", err)
}
wg.Wait()
if !ok {
t.Errorf("UDP server read error: %v. Bytes read: %v", serverReadError, bytesRead)
}
}