-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031
base: master
Are you sure you want to change the base?
feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031
Conversation
Ah, just realized I missed a changelog entry 😅, will add in a moment. |
5d16838
to
7758e8f
Compare
Added two changelog entries in the latest force push. |
…eate Problem: Prior to this commit it was not possible to specify the log group's class type. The method prior always created a Standard type. ------------------------------------------------------------ Solution: Allow specifying the log group class type via a new field, `group_class` which takes over the `create_missing_group`. deprecation: `create_missing_group` Initial Issue Report: vectordotdev#22008 Closes vectordotdev#22008
7758e8f
to
10fbf33
Compare
Latest force push caught a potential regression, realized I accidentally broke support for the old behavior supported by Now if |
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.
Thanks for this contribution @PriceHiller !
I think it isn't immediately apparent that specifying group_class
will cause a group to be created. What would you think of leaving in create_missing_group
without deprecation and have that control whether a group is created. group_class
could then default to standard
and only be used if create_missing_group
is true. I realize it is a bit of redundancy, but I think the options will be more discoverable.
I think you're right, what you've proposed is probably a better approach. I can modify this PR tomorrow (hopefully) to support that form of functionality instead. Will ping when done, for now I'll convert this PR to a draft (unless you'd prefer a new PR and me to close this). |
Summary
Copied from the body of commit:
This PR allows the specification of the log class to choose instead of defaulting to only the Standard log class type.
I'm not sure the approach I've taken is ideal, and I'm unsure as to the full deprecation procedure in code for the
create_missing_group
field.If the approach taken here with mirroring the
LogClassGroupDef
is not idiomatic/poor, I'm open to suggestions (and for anything else, of course) 🙂.Change Type
Is this a breaking change?
How did you test this PR?
cargo test --no-default-features --features "sinks-console,sinks-aws_cloudwatch_logs,aws-cloudwatch-logs-integration-tests" -- --skip 'sinks::aws_cloudwatch_logs::integration_tests' 'sinks::aws_cloudwatch_logs'
Makefile
.Does this PR include user facing changes?
Checklist
Cargo.lock
), pleaserun
dd-rust-license-tool write
to regenerate the license inventory and commit the changes (if any). More details here.References
create_missing_group
#22008