Address review comments

plugins/inputs/webhooks/papertrail/README.md

@@ -4,19 +4,18 @@ Enables Telegraf to act as a [Papertrail Webhook](http://help.papertrailapp.com/
 
 ## Events
 
-See [here](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#callback) for full documentation.
+[Full documentation](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#callback).
 
 Events from Papertrail come in two forms:
 
-* The event-based callback (shown in the example
-  [here](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#callback)):
+* The [event-based callback](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#callback):
 
-  * A point is created per event, with the timestamp as "received_at"
-  * Each point has a field counter (`count`), which is set to 1 (signifying the event occurred)
+  * A point is created per event, with the timestamp as `received_at`
+  * Each point has a field counter (`count`), which is set to `1` (signifying the event occurred)
   * Each event "hostname" object is converted to a `host` tag
   * The "saved_search" name in the payload is added as an `event` tag
 
-* The count-based callback (described [here](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#count-only-webhooks))
+* The [count-based callback](http://help.papertrailapp.com/kb/how-it-works/web-hooks/#count-only-webhooks)
 
   * A point is created per timeseries object per count, with the timestamp as the "timeseries" key (the unix epoch of the event)
   * Each point has a field counter (`count`), which is set to the value of each "timeseries" object
@@ -29,5 +28,5 @@ track the number of events by host and saved search.
 When an event is received, any point will look similar to:
 
 ```
-papertrail,host=myserver.example.com,event=saved_search_name count=3 1453248892
+papertrail,host=myserver.example.com,event=saved_search_name count=3i 1453248892000000000
 ```

plugins/inputs/webhooks/papertrail/papertrail_test.go

@@ -8,28 +8,82 @@ import (
 	"testing"
 
 	"github.com/influxdata/telegraf/testutil"
+	"github.com/stretchr/testify/require"
 )
 
-func postWebhooks(pt *PapertrailWebhook, payloadBody string) *httptest.ResponseRecorder {
-	req, _ := http.NewRequest("POST", "/", strings.NewReader(payloadBody))
+const (
+	contentType = "application/x-www-form-urlencoded"
+)
+
+func post(pt *PapertrailWebhook, contentType string, body string) *httptest.ResponseRecorder {
+	req, _ := http.NewRequest("POST", "/", strings.NewReader(body))
+	req.Header.Set("Content-Type", contentType)
 	w := httptest.NewRecorder()
-	w.Code = 500
 	pt.eventHandler(w, req)
-
 	return w
 }
 
+func TestWrongMethod(t *testing.T) {
+	var acc testutil.Accumulator
+	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
+	form := url.Values{}
+	form.Set("payload", sampleEventPayload)
+	data := form.Encode()
+
+	req, _ := http.NewRequest("PUT", "/", strings.NewReader(data))
+	req.Header.Set("Content-Type", contentType)
+	w := httptest.NewRecorder()
+	pt.eventHandler(w, req)
+
+	require.Equal(t, http.StatusMethodNotAllowed, w.Code)
+}
+
+func TestWrongContentType(t *testing.T) {
+	var acc testutil.Accumulator
+	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
+	form := url.Values{}
+	form.Set("payload", sampleEventPayload)
+	data := form.Encode()
+
+	resp := post(pt, "", data)
+	require.Equal(t, http.StatusUnsupportedMediaType, resp.Code)
+}
+
+func TestMissingPayload(t *testing.T) {
+	var acc testutil.Accumulator
+	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
+
+	resp := post(pt, contentType, "")
+	require.Equal(t, http.StatusBadRequest, resp.Code)
+}
+
+func TestPayloadNotJSON(t *testing.T) {
+	var acc testutil.Accumulator
+	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
+
+	resp := post(pt, contentType, "payload={asdf]")
+	require.Equal(t, http.StatusBadRequest, resp.Code)
+}
+
+func TestPayloadInvalidJSON(t *testing.T) {
+	var acc testutil.Accumulator
+	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
+
+	resp := post(pt, contentType, `payload={"value": 42}`)
+	require.Equal(t, http.StatusBadRequest, resp.Code)
+}
+
 func TestEventPayload(t *testing.T) {
 	var acc testutil.Accumulator
 	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
-	payload := url.QueryEscape(sampleEventPayload)
-	resp := postWebhooks(pt, payload)
-	if resp.Code != http.StatusOK {
-		t.Errorf("POST new_item returned HTTP status code %v.\nExpected %v", resp.Code, http.StatusOK)
-	}
+
+	form := url.Values{}
+	form.Set("payload", sampleEventPayload)
+	resp := post(pt, contentType, form.Encode())
+	require.Equal(t, http.StatusOK, resp.Code)
 
 	fields := map[string]interface{}{
-		"count": 1,
+		"count": uint64(1),
 	}
 
 	tags1 := map[string]string{
@@ -41,25 +95,23 @@ func TestEventPayload(t *testing.T) {
 		"host":  "def",
 	}
 
-	t.Logf("%v", acc.Metrics)
 	acc.AssertContainsTaggedFields(t, "papertrail", fields, tags1)
 	acc.AssertContainsTaggedFields(t, "papertrail", fields, tags2)
 }
 
 func TestCountPayload(t *testing.T) {
 	var acc testutil.Accumulator
 	pt := &PapertrailWebhook{Path: "/papertrail", acc: &acc}
-	payload := url.QueryEscape(sampleCountPayload)
-	resp := postWebhooks(pt, payload)
-	if resp.Code != http.StatusOK {
-		t.Errorf("POST new_item returned HTTP status code %v.\nExpected %v", resp.Code, http.StatusOK)
-	}
+	form := url.Values{}
+	form.Set("payload", sampleCountPayload)
+	resp := post(pt, contentType, form.Encode())
+	require.Equal(t, http.StatusOK, resp.Code)
 
 	fields1 := map[string]interface{}{
-		"count": 5,
+		"count": uint64(5),
 	}
 	fields2 := map[string]interface{}{
-		"count": 3,
+		"count": uint64(3),
 	}
 
 	tags1 := map[string]string{
@@ -75,7 +127,7 @@ func TestCountPayload(t *testing.T) {
 	acc.AssertContainsTaggedFields(t, "papertrail", fields2, tags2)
 }
 
-const sampleEventPayload = `payload={
+const sampleEventPayload = `{
   "events": [
     {
       "id": 7711561783320576,
@@ -115,7 +167,7 @@ const sampleEventPayload = `payload={
   "min_id": "7711561783320576"
 }`
 
-const sampleCountPayload = `payload={
+const sampleCountPayload = `{
    "counts": [
      {
        "source_name": "arthur",

plugins/inputs/webhooks/papertrail/papertrail_webhooks.go

@@ -2,10 +2,8 @@ package papertrail
 
 import (
 	"encoding/json"
-	"io/ioutil"
 	"log"
 	"net/http"
-	"net/url"
 	"time"
 
 	"github.com/gorilla/mux"
@@ -19,53 +17,45 @@ type PapertrailWebhook struct {
 
 func (pt *PapertrailWebhook) Register(router *mux.Router, acc telegraf.Accumulator) {
 	router.HandleFunc(pt.Path, pt.eventHandler).Methods("POST")
-	log.Printf("I! Started the papertrail_webhook on %s\n", pt.Path)
+	log.Printf("I! Started the papertrail_webhook on %s", pt.Path)
 	pt.acc = acc
 }
 
 func (pt *PapertrailWebhook) eventHandler(w http.ResponseWriter, r *http.Request) {
 	if r.Method != "POST" {
-		http.NotFound(w, r)
+		http.Error(w, "Method Not Allowed", http.StatusMethodNotAllowed)
+		return
 	}
 
-	defer r.Body.Close()
-	reqBody, err := ioutil.ReadAll(r.Body)
-	if err != nil {
-		http.Error(w, "Invalid request", 400)
+	if r.Header.Get("Content-Type") != "application/x-www-form-urlencoded" {
+		http.Error(w, "Unsupported Media Type", http.StatusUnsupportedMediaType)
 		return
 	}
 
-	data, err := url.QueryUnescape(string(reqBody))
-	if err != nil {
-		http.Error(w, "Invalid request", 400)
+	data := r.PostFormValue("payload")
+	if data == "" {
+		http.Error(w, "Bad Request", http.StatusBadRequest)
 		return
 	}
 
 	var payload Payload
-	// JSON payload is x-www-form-urlencoded, remove this string when unmarshaling
-	remove := "payload="
-	if len(data) > 0 && data[0:len(remove)] == remove {
-		err = json.Unmarshal([]byte(data[len(remove):len(data)]), &payload)
-		if err != nil {
-			http.Error(w, "Unable to parse request body", 400)
-			return
-		}
-	} else {
-		http.Error(w, "Invalid request", 400)
+	err := json.Unmarshal([]byte(data), &payload)
+	if err != nil {
+		http.Error(w, "Bad Request", http.StatusBadRequest)
 		return
 	}
 
 	if payload.Events != nil {
 
 		// Handle event-based payload
 		for _, e := range payload.Events {
-			// FIXME: Duplicate event timestamps will overwrite each other
+			// Warning: Duplicate event timestamps will overwrite each other
 			tags := map[string]string{
 				"host":  e.Hostname,
 				"event": payload.SavedSearch.Name,
 			}
 			fields := map[string]interface{}{
-				"count": 1,
+				"count": uint64(1),
 			}
 			pt.acc.AddFields("papertrail", fields, tags, e.ReceivedAt)
 		}
@@ -82,11 +72,11 @@ func (pt *PapertrailWebhook) eventHandler(w http.ResponseWriter, r *http.Request
 				fields := map[string]interface{}{
 					"count": count,
 				}
-				pt.acc.AddFields("papertrail", fields, tags, time.Unix(int64(ts), 0))
+				pt.acc.AddFields("papertrail", fields, tags, time.Unix(ts, 0))
 			}
 		}
 	} else {
-		http.Error(w, "Invalid request", 400)
+		http.Error(w, "Bad Request", http.StatusBadRequest)
 		return
 	}
 

plugins/inputs/webhooks/papertrail/papertrail_webhooks_models.go

@@ -19,9 +19,9 @@ type Event struct {
 }
 
 type Count struct {
-	SourceName string       `json:"source_name"`
-	SourceID   int64        `json:"source_id"`
-	TimeSeries *map[int]int `json:"timeseries"`
+	SourceName string            `json:"source_name"`
+	SourceID   int64             `json:"source_id"`
+	TimeSeries *map[int64]uint64 `json:"timeseries"`
 }
 
 type SavedSearch struct {