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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sequence token for PutLogEvents #115

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

Conversation

a10waveracer
Copy link

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    On January 4, 2023, Amazon announced that the sequence token is no longer required for PutLogEvents. This PR updates the library to reflect this new change and also adds functionality to emulate createGroup for log streams.

  • What is the current behavior? (You can also link to an open issue here)
    The refreshSequenceToken() function gets called somewhat regularly throughout the code.

  • What is the new behavior (if this is a feature change)?
    No more refreshSequenceToken() or associated calls!

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    Based upon my initial testing, this does not cause a breaking change -- I was able to slide this in seamlessly without any issues in my environment. The AWS announcement notes that this change is available in all commercial regions which, I assume, means that ex. GovCloud doesn't have support for this? But the API Documentation doesn't reflect this limitation... 馃し
    This does open up the possibility for users to leverage the new createStream constructor option to skip the describeLogGroups calls (which can cause issues similar to fix AWS Limits Race-Condition聽#113 and, frankly, is the reason I'm doing this in the first place).

  • Other information:
    This is also addressed in request [FEATURE] Update to match changes in Cloudwatch聽#114.

@A-Shevchenko
Copy link

Just a quick comment - proposed PR would be a very nice addition, as it will be possible to omit calls to describeLogStreams, which by default limited to 5 reqs per second

@markinjapan
Copy link

markinjapan commented May 31, 2023

@a10waveracer Since this repository's owner hasn't been active recently, I've offered to fork and continue this repository in the meantime. This PR looks great, so I'd like to incorporate it into my fork: phpnexus/cwh.

If you recreated this PR against the above repository, I'd be happy to approve and merge it.

@markinjapan
Copy link

@a10waveracer FYI I have removed the use of sequence tokens and added a constructor option $createStream in v3.1.0 of my fork:
https://github.com/phpnexus/cwh/releases/tag/v3.1.0

@a10waveracer
Copy link
Author

@markinjapan that's phenomenal -- thanks for the heads up!

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

3 participants