-
Notifications
You must be signed in to change notification settings - Fork 680
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
base: master
Are you sure you want to change the base?
json adapter #65
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" | ||
"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}}", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing ".", should be "{{.Container.Name}}". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is intentional. |
||
} | ||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Of course, this is more about semantics and code readability. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{} { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for re-stating assumptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, it wasn't meant to be offending. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely disagree.
There was a problem hiding this comment.
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.