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

5 second batching seems to only allow for messages every 5 seconds #10

Open
matthalladay opened this issue Mar 23, 2016 · 13 comments
Open

Comments

@matthalladay
Copy link

This might be the intended usage of the hook, but I intend to potentially log messages faster than every five seconds and definitely don't want them aggregated. I just changed 5 to 0 in the code, but wanted to let you know.

@vincentserpoul
Copy link
Contributor

Should it be a param instead?
We could simply make it appear in a param of NewInfluxDBHook, as the rest of the default values

@matthalladay
Copy link
Author

I think that it 'should' batch them, but should queue/coalesce for the
prescribed time period and then 'write multiple points" to influx as
described in
https://docs.influxdata.com/influxdb/v0.11/guides/writing_data/. I would
not expect log messages to be dropped, even if they are happening one
right after the other.

From: Vincent Serpoul [email protected]
To: Abramovic/logrus_influxdb [email protected]
Cc: matthalladay [email protected]
Date: 03/23/2016 09:55 PM
Subject: Re: [logrus_influxdb] 5 second batching seems to only
allow for messages every 5 seconds (#10)

Should it be a param instead?
?
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub[github.com]

@abramovic
Copy link
Owner

What if we use a channel and include a timer that processes the batch every 5 seconds?

pointChannel := make(chan entry *logrus.Entry)

func (hook *InfluxDBHook) Fire(entry *logrus.Entry) error {
     pointChannel <- entry
     ...
     ...
     ...
     return nil
}
...
...
...
func (hook *InfluxDBHook) processEntries() error {

L:
for {
        select {
        case pt, open := <-pointChannel:
            if !open {
                break L
            }
            hook.batch.AddPoint(pt)

            // when the batch size gets too large, write to adapter
            if len(hook.batch) >= MAX_SIZE {
                processBatch(hook.batch)
            }

        case time.NewTicker(time.Millisecond * 5000):
                        processBatch(hook.batch)

        default:
            // do nothing
        }
    }
return nil
}

@vincentserpoul
Copy link
Contributor

@matthalladay: what you describe is what it is supposed to do, are you saying it doesn't "queue" points and then write multiple points? I've used this code for a while on my side and didn't see any dropped points. Maybe I didn't pay enough attention?
I will check more carefully, add a test and add the nobatch option.

@abramovic , I'm not sure it would solve the issue, if there is one. I'm not sure what the issue is though :)

@abramovic
Copy link
Owner

@matthalladay @vincentserpoul
https://github.com/Abramovic/logrus_influxdb/tree/batching

/// This should remove remove the batching 
NewInfluxDBWithConfig(&Config{BatchInterval: 0}) 

@matthalladay
Copy link
Author

I'm sorry I'm a bit of a newb when it comes to Go, but I know what was
happening. The 5 second delay never gets triggered if you don't have
another message. The times are still accurate when the messages do get
triggered, but you can't see them. I'm going to try triggering a go
routine with a channel and a delay.

  • Matt

From: Vincent Serpoul [email protected]
To: Abramovic/logrus_influxdb [email protected]
Cc: matthalladay [email protected]
Date: 03/23/2016 09:55 PM
Subject: Re: [logrus_influxdb] 5 second batching seems to only
allow for messages every 5 seconds (#10)

Should it be a param instead?
?
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub[github.com]

@abramovic
Copy link
Owner

@vincentserpoul

I don't think this writes to InfluxDB every 5 seconds. It looks like it writes to InfluxDB if it's been more than 5 seconds since the last time a logrus entry was collected.

if hook.lastBatchUpdate.Add(hook.batchInterval).Before(time.Now()) ||
        len(hook.batchP.Points()) > 200 {
        err = hook.client.Write(hook.batchP)
        if err != nil {
            return fmt.Errorf("Fire: %v", err)
        }
        hook.lastBatchUpdate = time.Now()
        hook.batchP = nil
    }

Example:

  • 12:00 - first logrus entry is fired
  • ... (no data from logrus is collected)
  • ... (no data from logrus is collected)
  • 12:50 - second logurs entry is fired. The two logrus entries from 12:00 and 12:50 are both flushed to InfluxDB since it's been more than 5 seconds since 12:00

@abramovic
Copy link
Owner

@matthalladay - yes, I think that's what's happening with the current batching. I just made a pull request that should take care of that for you.

@vincentserpoul thoughts? #11

@matthalladay
Copy link
Author

Nick,

Thanks for making changes so quickly! There is another issue (not so much
with the hook as with how info is written to Influx). Because influx is
limited to storing unique points based on the time stamp, on my computer I
needed to add an additional delay of 100 nanoseconds. With this change,
we've done nothing with the overloaded constructor where you pass in your
own Client, you need to call stop. See below for the changes and an
implementation.

Changes:

Implementation:

  • Matt

From: Nick [email protected]
To: Abramovic/logrus_influxdb [email protected]
Cc: matthalladay [email protected]
Date: 03/24/2016 12:53 PM
Subject: Re: [logrus_influxdb] 5 second batching seems to only
allow for messages every 5 seconds (#10)

@matthalladay[github.com] - yes, I think that's what's happening with the
current batching. I just made a pull request that should take care of that
for you.
@vincentserpoul[github.com] thoughts? #11[github.com]
?
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub[github.com]

@abramovic
Copy link
Owner

The images didn't get attached. Can you upload the images to Github? #10

Most likely the default precision is being set to milliseconds. Are you using NewInfluxDBHook or NewWithClientInfluxDBHook?

@vincentserpoul
Copy link
Contributor

@matthalladay @abramovic
Sorry, I'm not on the same timezone, I missed most of the action. I see that you made some progress.
I now understand the issue.

I will implement the new updated lib and bring some feedback, most probably 👍

nice reactivity Nick!

@abramovic
Copy link
Owner

@matthalladay are you still having issues with the timestamps?

@abramovic
Copy link
Owner

@matthalladay - any update? Thanks!

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

No branches or pull requests

3 participants