-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add support for dynamic log groups using templating variables. #81
Add support for dynamic log groups using templating variables. #81
Conversation
hello when will this get review/merged? This is a feature we are looking for! |
Hey @cwyl02 , sorry its taking longer but we are on it. |
README.md
Outdated
* `log_group_name`: The name of the CloudWatch Log Group that you want log records sent to. | ||
* `log_stream_name`: The name of the CloudWatch Log Stream that you want log records sent to. | ||
* `log_stream_prefix`: Prefix for the Log Stream name. The tag is appended to the prefix to construct the full log stream name. Not compatible with the `log_stream_name` option. | ||
* `log_group_name`: The name of the CloudWatch Log Group that you want log records sent to. This value allows a template in the form of `${variable}`. See `log_stream_name` description for more. The app will attempt to create missing log groups, and will throw an error if it does not have access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be $(variable)
? From #78 conversation, we planned to replace ${ }
with $( )
. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was waiting for your team to provide feedback on the chosen variable identifiers. If y'all are good with #()
I'll go ahead and fix the doc. Is there any additional feedback before making a final commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We voted and we prefer $()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll switch it up and test before committing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a commit changes the identifiers. Updated the readme. Made some comments better. Fixed a few log lines and added a couple debug lines. If you guys don't mind I'll follow-up with a clean-up PR (no features).
Closes #46, closes #6, closes #24, closes #49.
Description of changes:
This builds on #78 (more commits to that branch), and can be merged after (making this diff smaller), or in leu of #78.
I create a new struct type called
Event
that carries the Event payload and metadata through the entire request process. Previously the code was passing around the log stream name, and now it passes around the Event which contains the stream name, group name, tag and log record. This makes the data easier to reason about.groups
map-tracker to theOutputPlugin
struct. Gets added to when new groups are created.logStream
struct, so each log stream has a pinned unique group.This config:
Turns into this (groups):
Streams Created:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.