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

Follow with filter #152

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Follow with filter #152

wants to merge 3 commits into from

Conversation

faiq
Copy link

@faiq faiq commented Apr 27, 2016

Okay so this is a combination of my last 2 pull requests: #149 and #151 and I think this provides really what an end user would want.

So let me describe the changes here:

  1. take in a ctx.Context rather than a time.Time

This allows more flexibility in use cases as described here:

For example, I am working on a daemon that forwards journald logs to an external logging service over http. The only reason I would ever want it killed is if a signal interrupt were given, so to do this I send a time.Now() from my signal handler into this channel to stop the program gracefully.

If I use a context, its more clear that I want to cancel processing of the logs rather than sending a time. Alternatively if someone wants to finish processing at a certain time, they can still do so. All they would have to do is send a ctx.Done() whenever the time hts

  1. Use a filter function to figure out what part of the journald log you want

It's very reasonable for someone to want the entire journal entry, rather than just the message field. In fact, we use the Send() method from the journal library https://github.com/coreos/go-systemd/blob/master/journal/journal.go#L75 to write custom log fields for our programs into journald. It's only suitable that we should be able to pull them out for later processing

  1. Send the journal entries with the filtered fields to msgs chan<- map[string]interface{}

This allows us to read all fields from the jorunal without worrying about the type and send them asynchronously without having to marshal and unmarshal the entries

Willing to close #151 in favor of this.

Thanks for all the help

@yifan-gu
Copy link
Contributor

Sorry for not being responsive, (was not cc'd, so I lost this PR in notification).
Will take a review.

// holds journal events to process. tightly bounded for now unless there's a
// reason to unblock the journal watch routine more quickly
events := make(chan int, 1)
pollDone := make(chan bool, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change the size to 0, and close the channel instead of sending a bool?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or better, can we reuse these channels ?

@yifan-gu
Copy link
Contributor

yifan-gu commented May 13, 2016

Ref rkt/rkt#2041 as this is an API break change.

cc @derekparker

@lucab
Copy link
Contributor

lucab commented Jul 13, 2016

@faiq ping :)

This stale PR has been pending for some time, do you still want to proceed on this? If so, can you please address @yifan-gu comments?

@faiq
Copy link
Author

faiq commented Jul 13, 2016

yikes sorry about being so non-responsive and thanks for the ping. it turns out we did not end up needing this functionality. I am more than happy to finish work on this or close it, depending on if you think this patch would be helpful or not.

errChan := make(chan error, 1)
go func() {
for {
kvMap := make(map[string]interface{})
Copy link
Contributor

@lucab lucab Jul 14, 2016

Choose a reason for hiding this comment

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

Why value type is interface{}? Aren't strings enough?

@lucab
Copy link
Contributor

lucab commented Jul 14, 2016

@faiq I think it would be nice to have some portions of this.

Point (1) looks good and point (3) makes the Follow() API more useful for the general case (even though I'm not sure why values are interfaces). We now have a GetEntry() which should help reducing the size of this PR.

Regarding point (2) I think the library shouldn't be concerned about this. And once the other two points are in place, it should be easy for consumers to just provide their own filtering wrapper.

@faiq
Copy link
Author

faiq commented Aug 4, 2016

Sorry I'm so slow at responding to this 😞 lots of work makes it hard to contribute to open source.

regarding why we're sending interface{} through the channel, this is because the values on the fields can be json by something like https://github.com/wercker/journalhook and logrus.

i can probably take out the enumerating logic for a call to GetEntry instead, and filter in go-land.

how does this all sound?

@lucab
Copy link
Contributor

lucab commented Aug 15, 2016

regarding why we're sending interface{} through the channel, this is because the values on the fields can be json by something like https://github.com/wercker/journalhook and logrus.

I'm not completely sure I'm understanding your proposal. Current API will return you either a string or a []byte, so even if the inner content is a JSON the receiver still need to unmarshal it. E.g. in the case of GetEntry below, this is already a map[string]string.

i can probably take out the enumerating logic for a call to GetEntry instead, and filter in go-land.

Just to be clear, who will be performing the filtering? Do you need go-systemd to filter fields, or will the receiver take care of that? I was suggesting the latter, but I may understand in some performance-related scenarios the former may be interesting too...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants