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.

Sign up for GitHub

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

Jump to bottom

feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(aws_cloudwatch_logs sink): allow setting type of log class to cr… #22031

wants to merge 1 commit into from

Conversation

PriceHiller
Copy link
Contributor

Summary

Copied from the body of commit:

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: #22008

Closes #22008

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

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

  • By running: 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'
  • NOTE: I was unable to run the integration tests for the cloudwatch logs. There doesn't appear to be a entry for them in the Makefile.

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

@PriceHiller PriceHiller requested review from a team as code owners December 13, 2024 21:12
@github-actions github-actions bot added domain: sinks Anything related to the Vector's sinks domain: external docs Anything related to Vector's external, public documentation labels Dec 13, 2024
@PriceHiller
Copy link
Contributor Author

Ah, just realized I missed a changelog entry 😅, will add in a moment.

@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 5d16838 to 7758e8f Compare December 13, 2024 21:19
@PriceHiller
Copy link
Contributor Author

Added two changelog entries in the latest force push.

@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 7758e8f to 10fbf33 Compare December 14, 2024 00:14
@PriceHiller
Copy link
Contributor Author

PriceHiller commented Dec 14, 2024 •
edited
Loading

Latest force push caught a potential regression, realized I accidentally broke support for the old behavior supported by create_missing_group.

Now if create_missing_group is enabled, we set the LogGroupClass to Some(LogGroupClass::Standard) which reflects the previous behavior.

jszwedko
jszwedko reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@jszwedko jszwedko left a 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.

@PriceHiller
Copy link
Contributor Author

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).

@PriceHiller PriceHiller marked this pull request as draft December 18, 2024 05:16
@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 10fbf33 to ccb5d3a Compare December 20, 2024 08:50
@github-actions github-actions bot removed the domain: external docs Anything related to Vector's external, public documentation label Dec 20, 2024
@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from ccb5d3a to 9e6a52b Compare December 20, 2024 09:05
@github-actions github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Dec 20, 2024
@PriceHiller PriceHiller marked this pull request as ready for review December 20, 2024 09:07
@PriceHiller PriceHiller requested a review from jszwedko December 20, 2024 09:08
jszwedko
jszwedko approved these changes Dec 20, 2024
View reviewed changes
Copy link
Member

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @PriceHiller ! This looks good to me now.

@jszwedko jszwedko enabled auto-merge December 20, 2024 22:30
@PriceHiller
Copy link
Contributor Author

Huh, interesting seeing those failures in the CI, I guess I get to state the most dangerous words in the world: "it works on my machine™".

I hate to be this much of a pain in the ass for less than a hundred line change, I just realized I didn't include the updated Cargo.lock 🤦.

Will add in a moment, and modify PR items as necessary.

@PriceHiller
feat(aws_cloudwatch_logs sink): allow setting type of log class to cr…
a8f71da 
…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`.

Initial Issue Report: vectordotdev#22008

Closes vectordotdev#22008
auto-merge was automatically disabled December 21, 2024 00:11

Head branch was pushed to by a user without write access

@PriceHiller PriceHiller force-pushed the aws-cloudwatch-specify-group-class branch from 9e6a52b to a8f71da Compare December 21, 2024 00:11
@PriceHiller
Copy link
Contributor Author

@jszwedko I do apologize for all the back and forth, just updated the Cargo.lock and the aws dep for the cloudwatch logs.

I had been hanging on to an updated ahash and crc32c dep locally due to a breakage with the stdsimd feature on linux. I didn't want to be too much a bother and push those, but the lack of updating that Cargo.lock caught up to me.

Also ran the dd-rust-license-tool to check licenses.

@jszwedko
Copy link
Member

No worries! I kicked off CI again. Thanks for the additional changes!

@jszwedko jszwedko enabled auto-merge December 21, 2024 05:09
@PriceHiller
Copy link
Contributor Author

I'm unsure if those latest errors in the CI are things I can resolve.

Appears to be a result of a missing API key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aliciascott aliciascott aliciascott approved these changes

@jszwedko jszwedko jszwedko approved these changes

Assignees
No one assigned
Labels
domain: external docs Anything related to Vector's external, public documentation domain: sinks Anything related to the Vector's sinks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AWS CloudWatch Logs - create infrequent access type with create_missing_group
3 participants
@PriceHiller @jszwedko @aliciascott