Add TCP support for writing logs #9

ghislainbourgeois · 2017-06-02T15:24:28Z

This change adds a way to send GELF logs over TCP. It also includes unit tests targeting a new tcpreader. The tcpreader's API is limited to the package because it will not handle multiple concurrent connections. I additionally tested it against a Graylog 2.2.1-1 instance running in docker.

mariussturm · 2017-06-02T15:32:41Z

That's awesome! Will review it in the next days!

mariussturm · 2017-06-14T15:51:38Z

gelf/tcpwriter.go

+func (w *TCPWriter) writeToSocketWithReconnectAttempts(zBytes []byte) (n int, err error) {
+	var errConn error
+	w.mu.Lock()
+	for i := 0; n <= 3; i++ {


Why does this loop always run three times? What happens when the first write is successful? The No. of re-attempts should be configurable. And a reconnect should be logged.

Good points, I'll update that portion.

I fixed the logic and made the maximum number of reconnection attempts configurable. I am uncertain how the logging should work however. Simply returning the number of bytes and an error would probably be considered by the client as a failure. Changing the method signature would mean not implementing the io.Writer interface. Do you have an idea I might have missed?

Yeah, that's a problem. Let's skip that one for now. Maybe we see a solution in a later stage.

mariussturm · 2017-06-21T15:10:09Z

I think the reconnection logic is not that easy. First I think we should wait for some time till the next attempt is triggered (also configurable?). Another issue happens when you close the server side while the writer tries to send:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x504e71]

goroutine 1 [running]:
github.com/Graylog2/go-gelf/gelf.(*TCPWriter).writeToSocketWithReconnectAttempts(0xc4200bc000, 0xc420076400, 0xad, 0x400, 0x0, 0x0, 0xc4200ac410)
	/home/mari/Repositories/torch/go/src/github.com/Graylog2/go-gelf/gelf/tcpwriter.go:80 +0x71
github.com/Graylog2/go-gelf/gelf.(*TCPWriter).WriteMessage(0xc4200bc000, 0xc4200d8300, 0x50, 0xc420070828)
	/home/mari/Repositories/torch/go/src/github.com/Graylog2/go-gelf/gelf/tcpwriter.go:50 +0x92
github.com/Graylog2/go-gelf/gelf.(*TCPWriter).Write(0xc4200bc000, 0xc4200ac410, 0x25, 0x50, 0x25, 0x0, 0x0)
	/home/mari/Repositories/torch/go/src/github.com/Graylog2/go-gelf/gelf/tcpwriter.go:66 +0x170
io.(*multiWriter).Write(0xc420098320, 0xc4200ac410, 0x25, 0x50, 0x6000a0, 0x0, 0x0)
	/usr/local/go/src/io/multi.go:60 +0x72
log.(*Logger).Output(0xc4200ac0f0, 0x2, 0xc4200d4280, 0x10, 0x0, 0x0)
	/usr/local/go/src/log/log.go:168 +0x210
log.Printf(0x556078, 0x10, 0x0, 0x0, 0x0)
	/usr/local/go/src/log/log.go:291 +0x80
main.main()
	/home/mari/Repositories/torch/go/src/github.com/Graylog2/go-gelf/test.go:33 +0xe7
exit status 2

mariussturm · 2017-06-21T15:11:39Z

gelf/udpwriter.go

@@ -57,9 +61,9 @@ func numChunks(b []byte) int {
 // New returns a new GELF Writer.  This writer can be used to send the
 // output of the standard Go log functions to a central GELF server by
 // passing it to log.SetOutput()
-func NewWriter(addr string) (*Writer, error) {
+func NewWriter(addr string) (*UDPWriter, error) {


For consistency I would call this NewUDPWriter. That's a breaking change but it's a major update anyway.

ghislainbourgeois · 2017-06-26T13:44:10Z

I tried to reproduce the bug you found an was unable to. Did you have code that you can share to reproduce or just a specific procedure?

mariussturm · 2017-06-26T13:51:02Z

Sure, I basically took the example from the Readme and added a end-less loop:

package main

import (
  "flag"
  "github.com/Graylog2/go-gelf/gelf"
  "io"
  "log"
  "os"
  "time"
)

func main() {
  var graylogAddr string

  flag.StringVar(&graylogAddr, "graylog", "", "graylog server addr")
  flag.Parse()

  if graylogAddr != "" {
    gelfWriter, err := gelf.NewTCPWriter(graylogAddr)
    if err != nil {
      log.Fatalf("gelf.NewWriter: %s", err)
    }
    // log to both stderr and graylog2
    log.SetOutput(io.MultiWriter(os.Stderr, gelfWriter))
    log.Printf("logging to stderr & graylog2@'%s'", graylogAddr)
  }

  // From here on out, any calls to log.Print* functions
  // will appear on stdout, and be sent over UDP to the
  // specified Graylog2 server.

  for {
    log.Printf("Hello gray World")
    time.Sleep(1 * time.Second)
  }

  // ...
}

Start a Graylog GELF input, let the program run and while it's sending logs stop the input.

mariussturm · 2017-06-29T10:48:14Z

Any luck in reproducing this?

ghislainbourgeois · 2017-06-29T11:13:27Z

Not yet, I have not had the time to work on this this week, but I think I have enough to go on for this weekend.

ghislainbourgeois · 2017-07-03T00:20:26Z

I was able to reproduce on commit 204855d but not on commits since the reconnection delay has been added. I also found a small error where I was applying the reconnect delay before all writes instead of before the reconnects.

0cjs · 2017-07-11T07:48:23Z

Actually, adding TCP and TLS support has been sort of bouncing on and off my to-do list for a while. I did fix up the docs to make it clear that GELF isn't restricted to UDP and clarified a few other things (PR #9); it may be worth rebasing this PR on to that but I also have no problem with re-doing my commit on that PR off of this, if this wants to go in first.

I've had only a quick look through these commits and I'm certainly not an expert Go programmer, but one comment I have is that I'd find this a lot easier to review if it were rebased to a) separate formatting changes and b) remove the things that were done and then undone later. Commit e98b8fb provides several examples of things that could be done to make the PR easier to read and review.

The go fmt run, if done as a completely separate commit, lets people skip through that separate commit (knowing it's just formatting changes) and the remaining parts that actually make changes generate a much smaller diff that focuses on code that actually needs to be reviewed.
Taking the CompressionType etc. out of the GelfWriter struct undoes the work done five commits earlier in 30ac897, also part of this PR, to add it in. If you go back and rewrite the earlier commit to do the right thing in the first place reviewers then don't have to waste time reading, thinking about and commenting on it being done the wrong way in that commit, and also now there's no later fix commit to also be reviewed.

My suggestion would actually be to take all the refactorings that touch the original code and make them the first commits on the PR; then the actual functionality changes will fall into the place prepared for them in a way that everything is exactly what's needed to add the new functionality without extraneous other stuff.

Oh, one other thing: I notice that the tests have gotten really slow (less than 1/10 second to over 8 seconds on my i7 system). It would be really nice to fix that; I'll have a look as I do further review and see if I see anything that might help there.

BTW, feel free to e-mail me to work out where to do text chat (I prefer Gitter) if you want to talk back-and-forth.

mariussturm · 2017-07-11T08:54:11Z

Agree with @C-J-S, I would love to merge this but the final review is a little bit hard in the state the PR is right now.

ghislainbourgeois · 2017-07-11T14:12:25Z

Makes sense, I will work on rebasing the current work. Should I force push to this branch or open a new PR once done?

For the test being slow, it is because the tests for TCP sleep a while. The tcpreader code that is used for the test has some synchronization issues that I was not able to fix yet. All that code is not exported however.

I also joined the Graylog2 room on Gitter for further chats.

mariussturm · 2017-07-11T15:33:35Z

@ghislainbourgeois you can force push to this PR. But like @C-J-S said, having one PR for refactoring work and one for the new feature would be great. In that way we can see more quickly if we have to think about some edge cases. I can also need some help on testing this, especially when this should go into Docker it needs a certain quality.

0cjs · 2017-07-11T15:44:07Z

Oh, I wasn't suggesting a separate PR for refactoring work! When refactoring in preparation for adding new features, refactorings are often one that you otherwise wouldn't do or even might actively object to. A bunch of holes in your lawn is only desirable if you know that they'll be filled with fence posts in a few days. :-)

I think it's fine to force push to this PR. If you're worried about being able to reference back to this work, you can always create another branch in your own repo to maintain a pointer to these commits until you feel you don't need it any more.

I will try to have another look at this tomorrow. One thing I noticed is that the code currently doesn't allow multiline messages (the complete message passed in is sent as full_message and only the first line of it is stored as message) which I find somewhat annoying because it makes it hard to have, e.g., a three-line error in message and a thirty-line stack trace in full_messsage or whatever. If we're changing the API here anyway, we may want to have a look at things like that, too.

FWIW, my use case for this is with SystemdJournal2Gelf to export log messages from various Unix systems into Graylog while maintaining more metadata than rsyslogd can manage. I expect to have messages sometimes hundreds of KB long.

mariussturm · 2017-07-12T08:36:49Z

@C-J-S oh sorry, misread that. Could you do some testing with SystemdJournal2Gelf and the latest commits? That would be great help.

0cjs · 2017-07-13T12:14:20Z

@mariussturm: Sorry for taking so long to get back to you. Yes! Your new commits are much better. The refactoring looks nice and the interface change and the TCP code are nicely split.

My previous concerns about the interface were due to a misreading of the SystemdJournal2Gelf code; my issues with message vs. fullMessage can all be dealt with in that. So the one thing I want to do is a quick review of our current Message struct here vs. the GELF spec and see if there are any other interface changes we might want to introduce while we're breaking backward compatibility anyway. (MIght as well do it all at once.)

However, the tests failed when I ran them. Are they failing for you?

I will try, in the next day or two, to look at the current testing framework and see if it can be made to work faster and better, and also convert SystemdJournal2Gelf to use the new library and see how it works out.

ghislainbourgeois · 2017-07-13T13:43:09Z

@C-J-S My bad for the tests, I will fix it and redo the rebase the same way, I forgot a small change.

ghislainbourgeois · 2017-07-15T03:19:11Z

I was able to figure out how to speed up the tests and make them a bit more robust and I refactored them a little at the same time. The tests now take about a second on my machine instead of 8.

0cjs · 2017-07-18T06:20:46Z

That's great. The test cleanup is nice. Looks like all those changes can be squashed into the Add TCP transport support commit, right? (Again, to make review easier, since we won't have to review the slower version of the tests, etc.)

This just assumes that the `github.com/c-j-s/go-gelf/gelf` working copy is set to the branch above. I'm not clear on how one might ask for a specific version of a package in Go. Perhaps make it part of the working copy name? For details see: Graylog2/go-gelf#9

ghislainbourgeois · 2017-07-18T11:36:47Z

Yes, I did not want to rebase before you had a chance to be made aware in case you started work on the previous commit. It is now rebased.

0cjs · 2017-07-18T15:59:57Z

Oh, you should always feel free to rebase away; I'll still have all your old commits in my repo and I can simply rebase any work I've done based on your old commits onto the new version of your branch at whatever point it's convenient to me.

I should be able to get some work done on SystemdJournal2Gelf tomorrow.

Looking back at the API stuff, I realize now the bit that concerned me slightly actually is in go-gelf: the Write([]byte) interface. I'm not sure, when writing to Graylog, that splitting the bytestring given to that into separate bits for message and fullMessage if the bytestring is more than one line long is really the right thing to do, since Graylog handles a multiline message just fine. That said; I don't have a strong opinion about it since I'm not very familiar with how people generally use Graylog and it's not an API I would use much anyway.

iporsut · 2017-07-18T23:35:11Z

gelf/message.go

+
+// Syslog severity levels
+const (
+	LOG_EMERG   = int32(0)


You could declare const like this.

const ( LOG_EMERG int32 = iota LOG_ALERT LOG_CRIT LOG_ERR LOG_WARNING LOG_NOTICE LOG_INFO LOG_DEBUG )

Maybe you unnecessarily to fix type of const because Go const can untype value.
So you can declare only value like this.

const ( LOG_EMERG = iota LOG_ALERT LOG_CRIT LOG_ERR LOG_WARNING LOG_NOTICE LOG_INFO LOG_DEBUG )

I agree this looks more idiomatic Go, and it still passes all the tests, however, I did not write that code, I simply moved it from the UDP code, so I cannot be sure if there was a reason for declaring the const like this.

I tested with a Graylog instance and this did not not break behaviour, so I applied the change and force pushed.

ghislainbourgeois · 2017-07-19T00:29:26Z

@C-J-S, regarding the Write(p []byte) interface, I am not sure either, we could keep it as is, put the full message in 'short_message', or change 'full_message' to contain everything but the first line.

According to the GELF documentation (http://docs.graylog.org/en/2.2/pages/gelf.html#gelf-payload-specification), 'full_message' can contain a backtrace, and 'short_message' is required and should be short. I understand from this that 'full_message' supports multiline but not 'short_message', but it is not completely clear.

Only referring to that documentation and the name of the fields, I would keep things as is.

If I am missing information, I would be willing to change the implementation, but would also suggest updating to documentation to clarify.

iporsut · 2017-07-19T00:38:55Z

gelf/message.go

+	return messageBytes, nil
+}
+
+func constructMessage(p []byte, hostname string, facility string, file string, line int) (m Message) {


I think return type of constructMessage could be pointer of Message m *Message. Then line 132 could be m = &Message{ and caller side can use without &m again.

Agreed, I will make that change and rebase it in the two separate commits.

You can certainly leave out the int32 declarations in the serverity levels because they're not int32s; the allowed range of the priority is 0 through 23. That said, I'd probably stick with assigning explicit numbers, as in

LOG_EMERG = 0 LOG_ALERT = 1 LOG_CRIT = 2 LOG_ERR = 3 ...

because these numbers for individual facilities are specifically (if informally) assigned and cannot be changed; I think doing it as above makes this more clear.

I would do this as a separate refactoring commit before your other changes to keep it small and clear about what's going on.

@C-J-S I just forced push that commit rebased before the rest.

0cjs · 2017-07-19T11:01:41Z

I've just added issue #11 with my take on the Write([]byte) interface. Feel free to discuss there. I suggest if we change that interface, we quickly code it up and get the PR done and merged, and then rebase this on it; that will minimize the amount of time/commits where we have an interface-breaking change that can't be caught by the compiler.

MiLk · 2017-07-20T03:15:13Z

README.md

@@ -52,7 +52,10 @@ giving us both centralized and local logs.  (Redundancy is nice).
 		flag.Parse()

 		if graylogAddr != "" {
-			gelfWriter, err := gelf.NewWriter(graylogAddr)


Why not keeping the NewWrite function which would return the gelf.Writer interface?
Depending on the string given to the NewWriter function, an UDPWriter or TCPWriter would be returned.
It would be easier to add new writers in the future.

Because the parameters are different, or ought to be, at any rate. A UDP connection has optional compression; TCP connections do not. TLS connections will need more parameters for the key and certificate information.

This can be achieved using functional options

Oh, I do like those functional options. That's basically a monoid with the operation being function application and the identity element being the identity function (that returns its input). (Well, but appropriately distorted to take into account that mutable variables are being passed around.) That kind of thing is a trick we use in Haskell.

That said, while I can see how this could be a handy trick for wrapping the TCP connection in TLS, since they're both stream connections:

I think we'd may still have the library supply a constructor for the TLS connectivity since applications other than SystemdJournal2Gelf may want to use TLS; and

I can't really see using a common constructor working for stream (TCP/TLS) and datagram (UDP) connections because the options are just so completely different.

The only thing that the stream and datagram connections share in common are the host and port. But datagram has chunking (implying a size limit users may want to set) and (optional) compression, neither of which stream connections allow. Stream connections potentially have all the TLS key and cert info, which datagram connections do not have.

Maybe change function name to Dial as net package https://golang.org/pkg/net/#Dial with functional options.

I was just thinking the other day, after reading about conventions for package names, that NewWriter was the wrong name anyway. Dial for consistency with other stuff sounded good until I did more research, and now I'm realizing that we seem to be going about this all wrong; GELF and its framing is all application-level protocol stuff and should just run over any stream or datagram connection, all of which we could have the client supply to us as something that implements Write([]byte) (len, error). Of course, we'd need some internal configuration to deal with setting appropriate framing based on whether we're told it's a stream or datagram (or "packet", as they say in net package world) connection. Things like reconnecting should be handled by a different package whose Writer the go-gelf package would use.

MiLk · 2017-07-20T03:19:16Z

gelf/message.go

+	LOG_DEBUG   = 7
+)
+
+func (m *Message) MarshalJSONBuf(buf *bytes.Buffer) error {


Any reason to have a custom MarshalJSONBuf function instead of implementing the standard Marshaler interface?

That's unchanged code from where the PR started; I think that it's probably better to move refactorings like that (not directly necessary to this PR) to a different PR.

Ah yes, you are right.

I think we should keep it, @dqminh did a lot of optimization there to get it as fast as possible.

@MiLk usage of MarshalJSONBuf is mostly because we want to accept an existing buffer from a pool or otherwise to save on an extra allocation.

Yep, got it. Thank you.

0cjs · 2017-07-21T08:50:57Z

An FYI: Yesterday @MiLk and I did a bunch of trial refactorings against this code; you can see them on my dev/cjs/170721/refactoring branch. These do some more API cleanup, among other things. I think it's best to sort them out a bit and do them against master branch and then see about putting the TCP stuff on the end of that. I hope the commits are reasonably clear now, but I will be clarifying futher later. (I was going to do at least some of that today but got too busy at work.)

Discussion of this in the go-gelf Gitter channel is welcome.

0cjs · 2017-07-26T14:36:07Z

I mentioned above that I have some new thoughts on what the architecture of this library should look like. I've written them up in this document. I'm not sure if here is the best place to discuss it; possibly we want to open a new issue or PR about this? (As usual, I welcome Gitter chat, too.)

(Side note: Yes, I did just change my GitHub account name from c-j-s to 0cjs; the former was just too much of a pain to type.)

mariussturm · 2017-07-26T14:58:00Z

@0cjs thanks for writing this up. As it looks like the refactoring is way bigger then the TCP support. So I think it's just fair to merge that first and than work on the architecture. @ghislainbourgeois is waiting for this a long time now.

0cjs · 2017-07-26T16:19:55Z

I'd like to see a merge as soon as possible, too, but I worry about the effect of API changes on clients. Perhaps we should start by declaring the current head of master version 1.0.0, starting a v1 branch, and making that the default branch. Then we can decide whether it's worth an API change for this PR and either put it as is on a new v2 branch, making the next API-incompatible change v3, or tweak back the (I think minor) things that made the API incompatible and make it version 1.1 on the v1 branch. I guess I tend towards the latter if we're thinking that my proposal for a big API change is a plausible idea, just to avoid putting clients through two API changes rather than one when the first one doesn't buy them anything if they don't want to use TCP.

And I'm really not clear on how the Go world deals with stuff like this. Maybe separate checkouts of different branches under, e.g., src/github.com/Graylog2/go-gelf/v{1,2}/ or something like that?

However you want to do it, it doesn't really much affect implementing my ideas; all the code is there to be pulled over and munged regardless of what branch it's on.

ghislainbourgeois · 2017-07-26T20:30:13Z

@0cjs @mariussturm, I also think the architecture refactoring is a nice endeavor, but it will take a while to get it done properly, forcing a good quantity of code in a diverging state for that period. However, Go does not support package versions at all, we need to have a different import path to release a new version.

I just found the GoPkg.in service that does url rewriting to branches or tags. With that service, we could merge the TCP code and create version tags and unique import paths while keeping a single repository. I tested it on my repository and it works fine. You simply have to name branches or tags with their naming convention and it is completely transparent. We could also keep the default branch to the earlier UDP-only version so existing client do not break.

This would let us do the big refactoring in small incremental steps that we could continually merge, rather than going for the perfect architecture in one step. What do you guys think?

MiLk · 2017-07-27T00:32:08Z

I also would like to see this PR merged before any refactor, even if it requires more work during the refactor.

I think the following plan is good:

Tagging the repo in its current state as v1.0.0
Merging this PR as v1.1.0 (shouldn't we keep the NewWriter function to not break exisiting app?)
Start the refactoring on a v2 branch.

GoPkg.in should support that correctly.

ghislainbourgeois · 2017-07-27T13:32:01Z

Unfortunately, I tested this morning and we can only target major version numbers, and the TCP feature has some breaking changes. Even if we keep NewWriter, Writer has changed from a struct to an interface. My suggestion is thus to use v1 for the UDP-only version, v2 for this PR and v3 for the refactoring of the API. PR #13 contains suggested changes to the documentation that should be reviewed and merged before this PR.

MiLk · 2017-07-28T00:19:41Z

Let's do that then.

I didn't see that we could use only major version numbers.

0cjs · 2017-07-28T00:46:56Z

Targeting only major version numbers for branches is just fine; if we use semantic versioning properly anybody using a particular version can safely upgrade to a later version with the same major version number (e g., 1.0.0 to 1.9.73) without changing their code in any way.

As for the return values, if my understanding of Go is correct there should be no problem maintaining interface compatibility. NewWriter, NewUDPWriter and NewTCPWriter all return structs, which are Writer, UDPWriter and TCPWriter respectively. Each of these has available the following interface (amongst all the various ones they make available):

{
    Write([]byte) (int, error)
    WriteMessage(m *Message) (err error)
}

So there is no problem with that interface that I can see, so long as Message and its associated functions have not changed (or can be changed back to what they originally were without problems).

It doesn't really affect me personally if we have one version or two versions of the API now, and two versions or three versions of the API later, but I think it would be nicer to clients of this library that aren't following along with all of this not to offer them two incompatible API changes in a relatively short period of time.

MiLk · 2017-07-28T00:52:48Z

gelf/tcpreader.go

+}
+
+func (r *TCPReader) readMessage() (*Message, error) {
+	r.okToRead.Lock()


This will create a dead lock if you write 2 messages on the same connection.

See https://github.com/ghislainbourgeois/go-gelf/pull/1

MiLk · 2017-07-28T01:15:37Z

gelf/tcpwriter_test.go

+func TestNewTCPWriter(t *testing.T) {
+	w, err := NewTCPWriter("")
+	if err == nil && w != nil {
+		t.Errorf("New didn't fail")


t.Error("New didn't fail")

MiLk · 2017-07-28T01:15:50Z

gelf/tcpwriter_test.go

+func TestNewTCPWriterConfig(t *testing.T) {
+	r, _, err := newTCPReader("127.0.0.1:0")
+	if err != nil {
+		t.Errorf("Could not open TCPReader")


t.Error("Could not open TCPReader")

MiLk · 2017-07-28T01:16:17Z

gelf/tcpwriter_test.go

+	signal <- "stop"
+	done := <-signal
+	if done == "done" {
+		return nil, fmt.Errorf("Wrong signal received")


errors.New("Wrong signal received")

MiLk · 2017-07-28T01:16:26Z

gelf/tcpwriter_test.go

+	signal <- "stop"
+	done := <-signal
+	if done == "done" {
+		return nil, fmt.Errorf("Wrong signal received")


errors.New("Wrong signal received")

MiLk · 2017-07-28T01:16:41Z

gelf/tcpwriter_test.go

+	signal <- "stop"
+	done := <-signal
+	if done == "done" {
+		return nil, nil, fmt.Errorf("Wrong signal received")


errors.New("Wrong signal received")

MiLk · 2017-07-28T01:43:00Z

@ghislainbourgeois What about having NewWriter defined as:

func NewWriter(addr string) (*UDPWriter, error) {
	return NewUDPWriter(addr)
}

Is there any other breaking change?

ghislainbourgeois · 2017-08-01T17:48:04Z

It will not really help because some clients like Docker specify *gelf.Writer. This is now an interface instead of a struct and needs to be referred to like so: gelf.Writer. Keeping NewWriter to dispatch to NewUDPWriter would not help in that case, so I am not sure it is worth it.

mariussturm · 2017-08-02T10:28:58Z

@ghislainbourgeois could you please rebase?

The severity levels are not int32, they are limited from 0-23.

This is a breaking change. Clients will need to use gelf.NewUDPWriter instead of gelf.NewWriter. The type returned will also change from *gelf.Writer to *gelf.UDPWriter, or gelf.Writer for the interface.

We expose a NewTCPWriter function that returns a TCPWriter type that implements the gelf.Writer interface.

By using a channel instead of a pair of mutexes and a slice of bytes attached to the reader. This also allows to write more than one message over the same connection.

ghislainbourgeois · 2017-08-02T12:29:32Z

@mariussturm I just rebased it

mariussturm · 2017-08-02T12:38:16Z

🎉 thanks a lot four your awesome work @ghislainbourgeois!!!

ghislainbourgeois · 2017-08-02T12:47:03Z

Thanks to you and all the reviewers!

0cjs · 2017-08-04T00:50:50Z

@mariussturm: It looks to me as if you did a squash commit when you brought this on to master, thus mushing five commits into one and making it significantly more difficult to review or audit the code in the future. (This is especially problematic for anybody who wants to go back and see what was done to add TCP, since the hundreds of lines of diff for that are now intermixed with many hundreds more lines of diff for the refactoring: in total there's about 2000 lines of diff in which to look to find any particular part of the change.

I feel pretty strongly that such destruction of history is a bad idea.

Can I suggest we drop this change and instead preserve the original commits by simply moving the master branch pointer up to the last commit on this PR? This is simply done with:

git fetch https://github.com/ghislainbourgeois/go-gelf.git
git branch -f master a9d0c42
git checkout master
git push --force-with-lease

As a side note, the reason you need to fetch from Ghislain's repo above is because the commits on this PR are not actually in the Graylog2 copy of this repo. Presumably if Ghislain deleted his repo the commits on this PR would simply be lost forever, except perhaps via this GitHub web interface.

0cjs · 2017-08-04T01:25:34Z

BTW, I also have some README fixes ready to PR, but I'd like to get this sorted out first.

mariussturm · 2017-08-04T08:14:56Z

@0cjs you can go back to this PR at any time to review the work. In the git history you want to have one commit per added feature. So squashing is a common practice. Otherwise you would have tons of commits in the history without a significant commit message. That's the reason why we want to have clear feature PRs without refactoring ten thinks on the side.

0cjs · 2017-08-04T08:38:00Z

The problem is, you can only review here in GitHub and you must know that a PR exists. When you work normally in your own clone of the Git repo none of this (the PR or its associated commits) is there. And also, this PR includes at least one commit that is nothing to do with the TCP addition (0f932fd).

At any rate, if you feel strongly that future people looking back through the code outside GitHub should not be able to see these changes except as one huge blob I guess that's something I'll just have to live with.

ghislainbourgeois mentioned this pull request Jun 2, 2017

GELF Driver does not support TCP moby/moby#33495

Closed

mariussturm reviewed Jun 14, 2017

View reviewed changes

mariussturm reviewed Jun 21, 2017

View reviewed changes

0cjs mentioned this pull request Jul 11, 2017

README: Fix errors in description and expand/clarify #10

Merged

iporsut reviewed Jul 18, 2017

View reviewed changes

iporsut reviewed Jul 19, 2017

View reviewed changes

0cjs mentioned this pull request Jul 19, 2017

Change Write([]byte) to log full message to message field #11

Open

MiLk reviewed Jul 20, 2017

View reviewed changes

ghislainbourgeois mentioned this pull request Jul 27, 2017

Update documentation to reflect default branch changes and GoPkg.in usage #13

Merged

MiLk reviewed Jul 28, 2017

View reviewed changes

MiLk approved these changes Aug 2, 2017

View reviewed changes

ghislainbourgeois and others added 5 commits August 2, 2017 08:23

Change severity levels to untyped

0f932fd

The severity levels are not int32, they are limited from 0-23.

Refactor UDP code to make room for TCP feature

61701ef

This is a breaking change. Clients will need to use gelf.NewUDPWriter instead of gelf.NewWriter. The type returned will also change from *gelf.Writer to *gelf.UDPWriter, or gelf.Writer for the interface.

Add TCP transport support

ee017f7

We expose a NewTCPWriter function that returns a TCPWriter type that implements the gelf.Writer interface.

Avoid deadlock in the tcp reader

a4c11f9

By using a channel instead of a pair of mutexes and a slice of bytes attached to the reader. This also allows to write more than one message over the same connection.

tcpreader: simplify the json decoding step

a9d0c42

mariussturm merged commit 25db870 into Graylog2:master Aug 2, 2017

Add TCP support for writing logs #9

Add TCP support for writing logs #9

Conversation

ghislainbourgeois commented Jun 2, 2017

mariussturm commented Jun 2, 2017

mariussturm Jun 14, 2017 • 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

mariussturm commented Jun 21, 2017

Choose a reason for hiding this comment

ghislainbourgeois commented Jun 26, 2017

mariussturm commented Jun 26, 2017

mariussturm commented Jun 29, 2017

ghislainbourgeois commented Jun 29, 2017

ghislainbourgeois commented Jul 3, 2017

0cjs commented Jul 11, 2017 • edited Loading

mariussturm commented Jul 11, 2017

ghislainbourgeois commented Jul 11, 2017

mariussturm commented Jul 11, 2017

0cjs commented Jul 11, 2017

mariussturm commented Jul 12, 2017

0cjs commented Jul 13, 2017

ghislainbourgeois commented Jul 13, 2017

ghislainbourgeois commented Jul 15, 2017

0cjs commented Jul 18, 2017

ghislainbourgeois commented Jul 18, 2017

0cjs commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghislainbourgeois commented Jul 19, 2017

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

0cjs commented Jul 19, 2017

MiLk Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0cjs Jul 25, 2017 • 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

0cjs commented Jul 21, 2017 • edited Loading

0cjs commented Jul 26, 2017

mariussturm commented Jul 26, 2017

0cjs commented Jul 26, 2017

ghislainbourgeois commented Jul 26, 2017

MiLk commented Jul 27, 2017

ghislainbourgeois commented Jul 27, 2017

MiLk commented Jul 28, 2017 • edited Loading

0cjs commented Jul 28, 2017

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

MiLk commented Jul 28, 2017

ghislainbourgeois commented Aug 1, 2017

mariussturm commented Aug 2, 2017

ghislainbourgeois commented Aug 2, 2017

mariussturm commented Aug 2, 2017 • edited Loading

ghislainbourgeois commented Aug 2, 2017

0cjs commented Aug 4, 2017

0cjs commented Aug 4, 2017

mariussturm commented Aug 4, 2017

0cjs commented Aug 4, 2017

mariussturm Jun 14, 2017 •

edited

Loading

0cjs commented Jul 11, 2017 •

edited

Loading

MiLk Jul 20, 2017 •

edited

Loading

0cjs Jul 25, 2017 •

edited

Loading

0cjs commented Jul 21, 2017 •

edited

Loading

MiLk commented Jul 28, 2017 •

edited

Loading

mariussturm commented Aug 2, 2017 •

edited

Loading