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

json adapter #65

Open
wants to merge 1 commit 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
139 changes: 139 additions & 0 deletions adapters/json/json.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
package json

import (
"encoding/json"
"errors"
"log"
"net"
"os"
"strconv"
"strings"
"text/template"

"github.com/gliderlabs/logspout/adapters/syslog"
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on another adapter should be avoided IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely disagree.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think separation of packages is of highest priority for a maintainable system, but as noted below it can be solved by moving/renaming the syslog message to some kind of base message where it's clear that it's supposed to be used by several adapters.

"github.com/gliderlabs/logspout/router"
)

var configDefaults = map[string]string{
"JSON_FIELDS": "time:uint,message,docker.hostname,docker.image",
"JSON_TIME": "{{.Time.Unix}}",
"JSON_MESSAGE": "{{.Data}}",
"JSON_SOURCE": "{{.Source}}",
"JSON_DOCKER_HOSTNAME": "{{.Container.Config.Hostname}}",
"JSON_DOCKER_IMAGE": "{{.Container.Config.Image}}",
"JSON_DOCKER_ID": "{{.Container.ID}}",
"JSON_DOCKER_NAME": "{{.ContainerName}}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing ".", should be "{{.Container.Name}}".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

func init() {
router.AdapterFactories.Register(NewJSONAdapter, "json")
}

func getopt(name string) string {
value := os.Getenv(name)
if value == "" {
value = configDefaults[name]
}
return value
}

type JSONAdapter struct {
conn net.Conn
route *router.Route
tmpl *template.Template
types map[string]string
}

func NewJSONAdapter(route *router.Route) (router.LogAdapter, error) {
transport, found := router.AdapterTransports.Lookup(route.AdapterTransport("udp"))
if !found {
return nil, errors.New("unable to find adapter: " + route.Adapter)
}
conn, err := transport.Dial(route.Address, route.Options)
if err != nil {
return nil, err
}

fields := strings.Split(getopt("JSON_FIELDS"), ",")
types := make(map[string]string)
var values []string
for _, field := range fields {
parts := strings.Split(field, ":")
if len(parts) > 1 {
types[parts[0]] = parts[1]
}
config := "JSON_" + strings.ToUpper(strings.Replace(parts[0], ".", "_", -1))
values = append(values, parts[0]+":"+getopt(config))
}
tmplStr := strings.Join(values, "\x00")
tmpl, err := template.New("prejson").Parse(tmplStr)
if err != nil {
return nil, err
}

return &JSONAdapter{
route: route,
conn: conn,
tmpl: tmpl,
types: types,
}, nil
}

func (a *JSONAdapter) Stream(logstream chan *router.Message) {
defer a.route.Close()
for message := range logstream {
m := syslog.NewSyslogMessage(message, a.conn)
Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems unnecessary to me, why create an instance of the syslog message just to render a template? It makes the code so much harder to read, especially when passing the connection. It's not idiomatic Go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, what is your suggested alternative. Second, it's sort of core to being able to specify new JSON fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using template.Execute() directly here and remove the dependency on syslog, or make the syslog message some kind of base message to use in several modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You understand that the dependency on the syslog adapter is no different than the dependency on the core router package, right? They are both core packages to the logspout project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, this is more about semantics and code readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion.

buf, err := m.Render(a.tmpl)
if err != nil {
log.Println("json:", err)
return
}
data, err := json.Marshal(buildMap(buf.String(), a.types))
if err != nil {
log.Println("json:", err)
return
}
_, err = a.conn.Write(data)
if err != nil {
log.Println("json:", err)
return
}
}
}

func buildMap(input string, types map[string]string) map[string]interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

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

It concerns me that the template rendering and value parsing happens on every log message. I think the goal of any forwarder is to be lightweight and performant, and leave the parsing to the receiver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Perhaps you can optimize this while maintaining configurable properties. For many this will be just fine. I encourage you to benchmark and show at what point this will not be viable to use.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I may do a benchmark if time allows, there will most likely be a big difference. For some people configurability matters more, for some raw performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for re-stating assumptions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it wasn't meant to be offending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I understand. I was getting caught up in style of communication. I appreciate your feedback and contributions, and any benchmarking around this will be very helpful to document. Not necessarily to prove a difference (since it will be obvious to anybody that reads the implementation), but to show what specific scenario this adapter should not be considered for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I'll let you know if I make a benchmark.

m := make(map[string]interface{})
fields := strings.Split(input, "\x00")
for _, field := range fields {
kvp := strings.SplitN(field, ":", 2)
keys := strings.Split(kvp[0], ".")
mm := m
if len(keys) > 1 {
for _, key := range keys[:len(keys)-1] {
if mm[key] == nil {
mm[key] = make(map[string]interface{})
}
mm = mm[key].(map[string]interface{})
}
}
var value interface{}
var err error
switch types[field] {
case "uint":
value, err = strconv.ParseUint(kvp[1], 10, 64)
case "int":
value, err = strconv.ParseInt(kvp[1], 10, 64)
case "float":
value, err = strconv.ParseFloat(kvp[1], 64)
case "bool":
value, err = strconv.ParseBool(kvp[1])
case "":
value = kvp[1]
}
if err != nil {
value = nil
}
mm[keys[len(keys)-1]] = value
}
return m
}
12 changes: 8 additions & 4 deletions adapters/syslog/syslog.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@ type SyslogAdapter struct {
func (a *SyslogAdapter) Stream(logstream chan *router.Message) {
defer a.route.Close()
for message := range logstream {
m := &SyslogMessage{message, a.conn}
m := NewSyslogMessage(message, a.conn)
buf, err := m.Render(a.tmpl)
if err != nil {
log.Println("syslog:", err)
return
}
_, err = a.conn.Write(buf)
_, err = a.conn.Write(buf.Bytes())
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there fixes to the syslog adapter in this PR? I suppose because of the dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a fix, it's part of a change required by this PR that helps generalize syslog.

if err != nil {
log.Println("syslog:", err)
return
Expand All @@ -100,13 +100,17 @@ type SyslogMessage struct {
conn net.Conn
}

func (m *SyslogMessage) Render(tmpl *template.Template) ([]byte, error) {
func NewSyslogMessage(message *router.Message, conn net.Conn) *SyslogMessage {
return &SyslogMessage{message, conn}
}

func (m *SyslogMessage) Render(tmpl *template.Template) (*bytes.Buffer, error) {
buf := new(bytes.Buffer)
err := tmpl.Execute(buf, m)
if err != nil {
return nil, err
}
return buf.Bytes(), nil
return buf, nil
}

func (m *SyslogMessage) Priority() syslog.Priority {
Expand Down
1 change: 1 addition & 0 deletions modules.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package main

import (
_ "github.com/gliderlabs/logspout/adapters/json"
_ "github.com/gliderlabs/logspout/adapters/raw"
_ "github.com/gliderlabs/logspout/adapters/syslog"
_ "github.com/gliderlabs/logspout/httpstream"
Expand Down