Use cached value from `sequenceToken`, if possible

[brunogsa]

Instead of using time to check if sequenceToken can be used, I just check if it's cached. If so, I just return it. Otherwise, I fetch it using AWS SDK.

I'm not sure why you were checking the time, since I didn't find anything on AWS SDK docs saying that sequenceToken should be refreshed.

Let me know if you see any problem in this new piece of code! Thanks!

[AppVeyorBot]

:white_check_mark: Build winston-aws-cloudwatch 396 completed (commit https://github.com/timdp/winston-aws-cloudwatch/commit/c92d0aac43 by @brunogsa)

[timdp]

I think the reasoning behind this was that you might have several clients pushing to the same log stream. If client A pushes log events, that will update the sequence token for the stream, and client B will need to refresh its token. I suppose the behavior could be optional, but I'm not sure it's a good idea to not refresh the token at all.

[brunogsa]

Sorry, I think I wrongly used the term "refreshed" in my PR description above.

The token is still being refreshed using the field sequenceToken, that is retrieved in every response of method putLogEvents from AWS SDK.

The cached value is always renewed, as soon as we have a new sequenceToken available.

However, the token doesn't expire. At least not by time. That's why I don't think we need to check any timestamp here.

Currently, the code has 2 ways of retrieving a new token:

Using method describeLogStreams

It returns the value nextToken that we can use to renew the token. See, however, as described in their docs, that this method has a limit of 5 transactions per second, which became a problem to me.

Using method putLogEvents

It returns the value sequenceToken that we can use to renew the token as well.

Conclusion

The idea behind my PR is calling describeLogStreams only when it's need (generally in the initial logs, where putLogEvents weren't called yet), so the limit of 5 transactions per second doesn't become a problem.

I have many clients pushing to the same Log Stream and got no problems regarding that.

I'm not sure what problems you're foreseeing here. :/ Could you explain that for me?

Thanks!

[timdp]

Thanks for the elaborate documentation!

I think I might be misunderstanding my own code. Let me take a look and get back to you. :slightly_smiling_face:

[timdp]

Okay, so it took me a while to respond. Sorry about that.

I think your PR might as well drop the whole this._sequenceTokenInfo in favor of a nullable this._sequenceToken. You're not using the date anymore, so there's no point in making it an object.

However, as I mentioned before, it misses one use case. If you have multiple clients pushing to the same stream, it's going to cause problems: a client that hasn't pushed for a while will keep trying to use its old sequence token, it'll never refresh it, and hence, it'll never be able to push anymore.

That was the reasoning behind the whole expiry feature. If you don't pass a maxSequenceTokenAge, by default, it'll request an up-to-date sequence token upon every push. If you set the maxSequenceTokenAge, every time it receives a new sequence token, it will refrain from requesting a new one until the current one expires.

In retrospect, perhaps it'd make a lot more sense to conditionally handle rejections from putLogEvents(). Judging by the docs, error.code will be InvalidSequenceTokenException upon concurrent access. I'll give it a try.

[brunogsa]

Makes sense!

Your approach of handling the rejection seems like a good solution to me.

Let's tweak the PR later to address it =]

[timdp]

I've already added it on master, but I still have to actually run it against CloudWatch rather than in tests before I publish a new major version. If you're feeling adventurous, feel free to already install from GitHub though. :wink:

[brunogsa]

Cool, thanks! 🙂

Feel free to close this PR if it's no longer necessary.

[timdp]

Took me a while again, but version 2.0.0 is now out! :tada: