Cloudwatch Logs producer #166

luqasz · 2017-07-09T12:17:15Z

Hi

I'd like to add cloudwatch logs producer to gollum. AWS puts some limitations on this service which are written in this plugin constants. I have couple of problems here.

Which type of base producer to use. AWS limits uploads to 5 / second.
I'd like to use templates for outgoing messages. Some one might want to send only syslog message itself and ignore stuff like priority and facility. I need to parse messages into a struct to be able to use templates. Any formatter can do that ?

Can you please give some advice / guide ?

coveralls · 2017-07-09T12:21:31Z

Coverage remained the same at 65.035% when pulling 66eff2d on luqasz:master into fe234d9 on trivago:master.

arnecls

This code does not compile.

Also:

Don't use "_" in filenames please. Go has special meanings for those postfixes. All lowercase, one word.
Please provide an integration test if possible

arnecls · 2017-07-10T08:06:42Z

producer/cloudwatch_logs.go

+// Configure initializes this producer with values from a plugin config.
+func (prod *CloudwatchLogs) Configure(conf core.PluginConfigReader) error {
+	prod.SetStopCallback(prod.flush)
+	stream := conf.GetString("stream", "")


Consider using struct tags for stream and group (non-pointer) and only leave the checks on empty in the config method

arnecls · 2017-07-10T08:07:51Z

producer/cloudwatch_logs.go

+	// do anything reasonable with rejected logs. Ignore it.
+	// Meybe expose some statistics for rejected counters.
+	resp, err := dst.svc.PutLogEvents(prod.cloudwatchLogsParams)
+	if err == nil {


if err != nil there should be an error message logged

arnecls · 2017-07-10T08:10:40Z

producer/cloudwatch_logs.go

+
+type CloudwatchLogs struct {
+	core.BufferedProducer `gollumdoc:"embed_type"`
+	stream                *string


Make these strings non-pointer.

It will make it easier to process your config

Strings are not scattered around in memory (cache locality, memory fragmentation, etc - won't hurt you here but in general I would consider it a good practice)

I've created them as pointers to easily set them in upload params.

arnecls · 2017-07-10T08:14:32Z

producer/cloudwatch_logs.go

+	}
+	err = prod.createStream()
+	return
+}


Fine like this, but you could also do it like this:

func (prod *CloudwatchLogs) create() error { if err := prod.createGroup(); err != nil { return err } return prod.createStream() }

I'll leave it up to you which version you like more.

arnecls · 2017-07-10T08:15:53Z

producer/cloudwatch_logs.go

+// http://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_CreateLogStream.html
+func (prod *CloudwatchLogs) createStream() error {
+	params := &cloudwatchlogs.CreateLogStreamInput{
+		LogGroupName:  &prod.group,


This should not compile ...?
LogGroupName is a *string, not a **string.

arnecls · 2017-07-10T08:19:54Z

producer/cloudwatch_logs.go

+	}
+}
+
+func (prod *Console) Produce(workers *sync.WaitGroup) {


this should be func (prod *CloudwatchLogs) ...

arnecls · 2017-07-10T08:20:10Z

producer/cloudwatch_logs.go

+func (prod *Console) Produce(workers *sync.WaitGroup) {
+	defer prod.WorkerDone()
+	prod.AddMainWorker(workers)
+	prod.MessageControlLoop(prod.printMessage)


printMessage function does not exist.
You probably want to call upload here?
If you use the buffered producer you will only get single messages though.
If you want to send batches of messages you should have a look at core.BatchProducer

arnecls · 2017-07-10T08:23:06Z

producer/cloudwatch_logs.go

+	"time"
+)
+
+/*


Please use // when doing comments.
Some people use /* .. */ to temporarily remove code and its super annoying if you have block comments inside your code that force you to create multiple blocks.

arnecls · 2017-07-10T08:24:08Z

producer/cloudwatch_logs.go

+// Put log events and update sequence token.
+// Possible errors http://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_PutLogEvents.html
+func (prod *CloudwatchLogs) upload() {
+	logevents := make([]*cloudwatchlogs.InputLogEvent, 0, len(events))


where does events come from?

coveralls · 2017-07-10T19:07:16Z

Coverage increased (+0.6%) to 65.655% when pulling f956fe1 on luqasz:master into fe234d9 on trivago:master.

luqasz · 2017-07-10T19:21:10Z

Where should I put code to:

get instance metadata (which will communicate with AWS external resources)
get token (which will communicate with AWS external resources)

It should be launched before first upload to cloudwatch. Is Produce() a good place ?

coveralls · 2017-07-10T19:22:11Z

Coverage increased (+0.6%) to 65.655% when pulling 0281b8a on luqasz:master into fe234d9 on trivago:master.

msiebeneicher

Please also check the current linter issues: https://travis-ci.org/trivago/gollum/jobs/252132692

msiebeneicher · 2017-07-11T13:18:56Z

producer/cloudwatchlogs.go

+	// PutLogEvents 5 requests/second/log stream.
+	putLogEventsDelay = 200 * time.Millisecond
+)
+


Please provide a documentation and a config example . Look here for an example: https://github.com/trivago/gollum/blob/master/producer/elasticsearch.go#L31

arnecls · 2017-07-12T12:29:48Z

producer/cloudwatchlogs.go

+
+type CloudwatchLogs struct {
+	core.BufferedProducer `gollumdoc:"embed_type"`
+	stream                string `config:"Stream" default:""`


"Stream" is quite close to the standard "Streams".
As by line 93 I would rename this to "LogStreamPrefix". Avoids ambiguity in the config.

arnecls · 2017-07-12T12:30:56Z

producer/cloudwatchlogs.go

+type CloudwatchLogs struct {
+	core.BufferedProducer `gollumdoc:"embed_type"`
+	stream                string `config:"Stream" default:""`
+	group                 string `config:"Group" default:""`


To align with my previous proposal I would rename this to "LogGroup".

arnecls · 2017-07-12T12:37:49Z

producer/cloudwatchlogs.go

+// Put log events and update sequence token.
+// Possible errors http://docs.aws.amazon.com/AmazonCloudWatchLogs/latest/APIReference/API_PutLogEvents.html
+func (prod *CloudwatchLogs) upload(msg *core.Message) {
+	logevents := make([]*cloudwatchlogs.InputLogEvent, 0)


You create logevents here and upload them later, but there is no data added to it ...?
Also this looks like you actually want to upload more than one event at a time.
As of this I would suggest you base this plugin on core.BatchProducer which gives you a list of messages instead of just one message.

I would suggest to have a look at producer.Kinesis for that.

arnecls · 2017-07-12T12:42:41Z

producer/cloudwatchlogs.go

+	if conf.GetString("group", "") == "" {
+		prod.Logger.Error("group name can not be empty")
+	}
+}


prod.service is not initialized at all.
If you need help about creation of the session and config object have a look at producer.Kinesis, producer.S3 or consumer.Kinesis (and of course the AWS SDK docs)

arnecls · 2017-07-12T12:49:34Z

Which type of base producer to use. AWS limits uploads to 5 / second.

Like written in my code comments - for AWS a core.BatchProducer is normally the best option as you get a batch of messages instead of single messages. As an advice - have a look at the other AWS producers (Kinesis and S3). Most of the common stuff is in there.

I'd like to use templates for outgoing messages. Some one might want to send only syslog message itself and ignore stuff like priority and facility. I need to parse messages into a struct to be able to use templates. Any formatter can do that?

Normally we use TextToJSON or write a custom formatter for that. After that you can use the different JSON processors (processJSON, templateJSON) to modify your message. This process will change with 0.6.0 (see #171) but for now this is how it works.

Also - please test your plugin on AWS and notify us when this actually works.
Until then I will mark this as "work in progress".

luqasz · 2017-07-12T21:23:06Z

Thx for your feedback. I will let you know when I am finished.

luqasz · 2017-07-18T21:15:46Z

How can I set default max batch size ? In kinesis producer dosc state that BatchMaxMessages is set to 500 but there is no code for that in producer/kinesis.go.

msiebeneicher · 2017-07-19T07:44:30Z

The batch settings comes from the embedded batchedproducer.
Here the embed producer: https://github.com/trivago/gollum/blob/master/producer/kinesis.go#L98
And here the settings comes from: https://github.com/trivago/gollum/blob/master/core/batchedproducer.go#L44

But the docs are not correct. The new configuration have to look like

Batch:
  MaxCount: 8192
  FlushCount: 4096
  TimeoutSec: 5

We will update all docs by #100

luqasz · 2017-07-19T09:01:17Z

Ok. Is there any way to set this in code ? I would like to set max limits based on AWS limits so they will not be exceeded by user configuring gollum.

coveralls · 2017-07-30T17:05:05Z

Coverage remained the same at 65.291% when pulling 0b8298b on luqasz:master into 2be9190 on trivago:master.

msiebeneicher · 2017-08-07T16:01:52Z

@luqasz:

The unit test fails because the TestProducerInterface try to instantiate all producers.
Because of the empty LogStream and LogGroup settings we get this error.

To fix this i would recommend this change to fix the current issue:

// Configure initializes this producer with values from a plugin config.
func (prod *AwsCloudwatchLogs) Configure(conf core.PluginConfigReader) {
	if prod.stream == "" {
		prod.Logger.Error("LogStream can not be empty")
	}
	if prod.group == "" {
		prod.Logger.Error("LogGroup can not be empty")
	}
	if conf.GetInt("Batch/MaxCount", maxBatchEvents) > maxBatchEvents {
		conf.Errors.Pushf("Batch/MaxCount must be below %d", maxBatchEvents)
	}
}

Please check the new guidelines of how we document the plugins now: http://gollum.readthedocs.io/en/latest/src/instructions/writingPlugins/docs.html
It is important to use the correct format because we auto-generate the documentation out of the code.

In this case the doc part should look like this example:

// AwsCloudwatchLogs producer plugin
//
// The AwsCloudwatchLogs producer plugin sends messages to
// AWS Cloudwatch Logs service.
//
// Parameters
//
// - LogStream: xxxx
//
// - LogGroup: xxx
//
// Examples
//
// This example ...:
//
//	CwLogs:
//		Type: AwsCloudwatchLogs:
//			LogStream: stream_name
//			LogGroup: group_name
//    Credential:
//      Type: shared
//

After the requested changes this PR should be mergeable! :)
thx for your work! 👍

luqasz · 2017-08-09T07:31:25Z

I will catch you later. I am busy now. Thx for reply.

coveralls · 2017-08-14T11:32:47Z

Coverage decreased (-0.02%) to 70.752% when pulling e714a05 on luqasz:master into 706e082 on trivago:master.

luqasz · 2017-09-05T19:47:08Z

Hi. Do you need any thing else from this PR ?

msiebeneicher · 2017-09-06T10:47:17Z

@luqasz : Sorry for the delay! We will merge this PR after the final v0.5.0 release. Last time I saw no further issues with this PR - so i think we are able to merge this directly after the release.

Handle errors in a separate function. Create group, stream if needed. Fixes errors when no group / stream is present when uploading.

luqasz · 2017-09-08T10:38:18Z

I've allowed myself to add a separate function to handle upload results. Usefull when group / stream was removed during program runtime. Gollum will create group / stream (without uploading batch) so next uploads will succeed.

coveralls · 2017-09-08T10:40:03Z

Coverage remained the same at 71.244% when pulling 3611427 on luqasz:master into 7feb8ac on trivago:master.

luqasz · 2018-02-13T19:48:12Z

@arnecls @msiebeneicher Do you need anything from me to merge this PR ?

arnecls · 2018-02-25T21:07:09Z

LGTM. If @msiebeneicher does not find anything I'll do the merge on Wednesday when I'm back at the office.

arnecls · 2018-02-28T16:42:09Z

Merged.
Thank you for contributing and for your patience 🍺

Will be released along with 0.5.2

Łukasz Kostka added 2 commits July 9, 2017 13:52

Initial cloudwatch logs producer

0b9bc5e

Merge branch 'master' of https://github.com/trivago/gollum

66eff2d

arnecls requested review from ppar, msiebeneicher and arnecls July 10, 2017 08:04

arnecls added the new feature label Jul 10, 2017

arnecls requested changes Jul 10, 2017

View reviewed changes

Łukasz Kostka added 5 commits July 10, 2017 20:24

Comment fix

4bafd8c

Remove unused code

d7d2a44

Fix Produce()

b1c612d

Shorten create()

6b4b56b

Make code compile

f956fe1

Use struct tags

0281b8a

msiebeneicher suggested changes Jul 11, 2017

View reviewed changes

Add plugin docs

24e6a44

arnecls requested changes Jul 12, 2017

View reviewed changes

arnecls added the WIP label Jul 12, 2017

Łukasz Kostka added 4 commits July 12, 2017 21:22

Add error handlong and logging

d80592a

Remove docs for removed format option

e542f5b

Add Region option for config

65f5379

Use dafault sdk credentials providers chain

1bfc70d

Łukasz Kostka added 3 commits July 30, 2017 18:56

Init service with AwsMultiClient

fc8f032

Docs update

adb9155

Merge remote-tracking branch 'origin/master'

0b8298b

msiebeneicher added pending-submitter-response and removed WIP labels Aug 7, 2017

msiebeneicher removed the request for review from ppar August 8, 2017 08:41

Łukasz Kostka added 3 commits August 14, 2017 12:59

Fix TestProducerInterface

eb82367

Merge remote-tracking branch 'origin/master'

29cbd35

Docs update

e714a05

msiebeneicher added this to the v0.6.0 milestone Aug 15, 2017

msiebeneicher added pending-maintainer-response and removed pending-submitter-response labels Aug 15, 2017

Łukasz Kostka added 2 commits September 8, 2017 12:19

Merge remote-tracking branch 'origin/master'

ff31377

Handle upload result function

3611427

Handle errors in a separate function. Create group, stream if needed. Fixes errors when no group / stream is present when uploading.

arnecls approved these changes Feb 25, 2018

View reviewed changes

arnecls removed the pending-maintainer-response label Feb 25, 2018

arnecls modified the milestones: v0.6.0, v0.5.x Feb 25, 2018

arnecls merged commit 3b7caa5 into trivago:master Feb 28, 2018

+              	"time"
+              )
+              /*

Cloudwatch Logs producer #166

Cloudwatch Logs producer #166

Conversation

luqasz commented Jul 9, 2017

coveralls commented Jul 9, 2017

arnecls left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 10, 2017

luqasz commented Jul 10, 2017

coveralls commented Jul 10, 2017

msiebeneicher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnecls commented Jul 12, 2017 • edited Loading

luqasz commented Jul 12, 2017

luqasz commented Jul 18, 2017

msiebeneicher commented Jul 19, 2017

luqasz commented Jul 19, 2017

coveralls commented Jul 30, 2017

msiebeneicher commented Aug 7, 2017

luqasz commented Aug 9, 2017

coveralls commented Aug 14, 2017

luqasz commented Sep 5, 2017

msiebeneicher commented Sep 6, 2017

luqasz commented Sep 8, 2017

coveralls commented Sep 8, 2017

luqasz commented Feb 13, 2018

arnecls commented Feb 25, 2018

arnecls commented Feb 28, 2018 • edited Loading

arnecls left a comment •

edited

Loading

arnecls commented Jul 12, 2017 •

edited

Loading

arnecls commented Feb 28, 2018 •

edited

Loading