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

syslog #23

Merged
merged 2 commits into from Jul 27, 2021
Merged

syslog #23

merged 2 commits into from Jul 27, 2021

Conversation

brandonbloom
Copy link
Member

@brandonbloom brandonbloom commented Jul 21, 2021

switch to syslog instead of log collector workers reading fifos

@brandonbloom brandonbloom requested a review from kendru July 21, 2021 23:37
@brandonbloom brandonbloom force-pushed the syslog branch 5 times, most recently from 8c9e922 to 9a878ae Compare July 26, 2021 21:37
@brandonbloom brandonbloom marked this pull request as ready for review July 26, 2021 21:37
@brandonbloom
Copy link
Member Author

@kendru please review.

@BenElgar if you have log handling issues on linux, please try this branch

chrono/chrono.go Show resolved Hide resolved
Store: store,
VarDir: paths.VarDir,
Store: store,
SyslogAddr: "localhost:4500", // XXX Configurable?
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should make this configurable. It looks like 4500 is commonly used for IPSec NAT traversal, so this might break some VPNs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I didn't check if this port was available. I'm not quite sure what the right thing to do here is. I actually think maybe we should pick a port at random and write it in the run state / config file thing. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is the right default behaviour, but it would still be nice to let users specify a specific port if they want to for whatever reason. This is another place where I think that it would be useful for exo to own the definition for the LogProvider. That way, there is one source of truth for the proxy to discover where to send logs.

logd/README.md Show resolved Hide resolved
logd/logd.go Show resolved Hide resolved
logd/logd.go Show resolved Hide resolved

"github.com/deref/exo/logd/api"
)

type Store interface {
// Returns the next log after the given argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to define what "next" means in this context. My immediate question is whether this would iterate through all logs available (regardless of what log I passed in first), or would or follow some natural ordering, potentially terminating without iterating all logs if I did not start at the "beginning".

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I'll improve this comment.

var nextName string
if err := sto.db.View(func(txn *badger.Txn) error {
opts := badger.DefaultIteratorOptions
opts.PrefetchSize = 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

This option is not used with PrefetchValues = false

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh. OK. Will remove.

it.Next()
if !it.ValidForPrefix(prefix) {
if it.Valid() {
nextName = string(it.Item().Key())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? If I'm reading this right, we are looking for a log with the same name as the key, but the key is `(log + <255>, id).

Copy link
Member Author

Choose a reason for hiding this comment

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

D'oh, you're right, this makes no sense at all. Will fix.

it := txn.NewIterator(opts)
defer it.Close()

// TODO: Something better than a linear key scan.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did somebody say secondary indexes? ;)

@@ -0,0 +1,169 @@
package logio
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of naming this logio, I think that we should go ahead and name it something more generic, since this is the component that will supervise processes. Maybe we should use something that conveys the idea of "supervisor", "proxy" or "wrapper" - foreman? bigbrother? cocoon? chrysalis? saran 😆?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, definitely needs a better name. I was thinking about just calling it supervise. That work?

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

Successfully merging this pull request may close these issues.

None yet

2 participants