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

Add AWS Cloudwatch Logs operator #289

Merged
merged 32 commits into from
May 18, 2021
Merged

Add AWS Cloudwatch Logs operator #289

merged 32 commits into from
May 18, 2021

Conversation

ericwholt
Copy link
Contributor

@ericwholt ericwholt commented May 5, 2021 •
edited by jsirianni
Loading

Description of Changes

Adds AWS Cloudwatch Logs operator

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Add a changelog entry (for non-trivial bug fixes / features)
  • CI passes

@ericwholt ericwholt requested a review from jsirianni May 5, 2021 13:03
jsirianni
jsirianni requested changes May 5, 2021
View reviewed changes
Copy link
Member

@jsirianni jsirianni left a comment

Choose a reason for hiding this comment

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

I tried to be thorough, so I left a lot of feedback. Some of which is opinionated or nitpicky.

operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
Eric Holt and others added 17 commits May 5, 2021 12:10
Remove references to azure in comments
4f93872
@ericwholt
Update operator/builtin/input/aws/cloudwatch/cloudwatch.go
81faa1a 
combine limit checks into one check.

Co-authored-by: Joseph Sirianni <joe.sirianni@bluemedora.com>
Change public functions to private
90c7251
Merge branch 'operator-aws-cloudwatch-logs' of github.com:observIQ/st…
2249855 
…anza into operator-aws-cloudwatch-logs
Fix formatting in limit check
a3d17e8
Do not set startTime defaults in CloudwatchInput
59e18f8
Update based on PR feedback and add parameters
856db12
Merge branch 'master' into operator-aws-cloudwatch-logs
0b5e6ae
go mod tidy
1e92d0a
Add constant for event limit with comment
57d3721
Return result of c.pollEvents instead of nil
1a7fd8d
Add cloudwatch documentation and remove timestamp from record
f473b6f
Update documentation and error message
d59e2be
Update log_group_name and region documentation
4c25a8f
Tidy up go mod
4d06bd1
Update handle batched events
ebffad4
Update debug message
4a4b060
@jsirianni jsirianni self-requested a review May 12, 2021 12:45
jsirianni
jsirianni requested changes May 12, 2021
View reviewed changes
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
ericwholt and others added 2 commits May 12, 2021 09:50
@ericwholt
Update docs/operators/aws_cloudwatch_input.md
08d057a 
Co-authored-by: Joseph Sirianni <joe.sirianni@bluemedora.com>
@ericwholt
Update docs/operators/aws_cloudwatch_input.md
a4220bd 
Co-authored-by: Joseph Sirianni <joe.sirianni@bluemedora.com>
@codecov
Copy link

codecov bot commented May 12, 2021 •
edited
Loading

Codecov Report

Merging #289 (ab11dc7) into master (54689dc) will decrease coverage by 1.25%.
The diff coverage is 30.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #289      +/-   ##
==========================================
- Coverage   70.62%   69.37%   -1.25%     
==========================================
  Files         117      119       +2     
  Lines        6096     6291     +195     
==========================================
+ Hits         4305     4364      +59     
- Misses       1326     1455     +129     
- Partials      465      472       +7
Impacted Files Coverage Δ
...perator/builtin/input/aws/cloudwatch/cloudwatch.go 28.19% <28.19%> (ø)
...builtin/input/aws/cloudwatch/cloudwatch_persist.go 100.00% <100.00%> (ø)
operator/builtin/output/otlp/otlp.go 62.96% <0.00%> (-2.47%) ⬇️
operator/builtin/output/newrelic/newrelic.go 71.96% <0.00%> (+0.93%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54689dc...ab11dc7. Read the comment docs.

Eric Holt added 3 commits May 12, 2021 14:48
Refactor for test and changes from feedback
ed76918
Return error on getEvents and Promote log_group_name, region, log_str…
195546e 
…eam_name to resource
Merge branch 'operator-aws-cloudwatch-logs' of github.com:observIQ/st…
841d1bd 
…anza into operator-aws-cloudwatch-logs

Merge upstream changes from suggestions with local changes
@ericwholt ericwholt requested a review from djaglowski May 12, 2021 21:27
@ericwholt ericwholt changed the title WIP initial AWS Cloudwatch Logs operator Add AWS Cloudwatch Logs operator May 12, 2021
@ericwholt ericwholt marked this pull request as ready for review May 12, 2021 21:37
djaglowski
djaglowski requested changes May 13, 2021
View reviewed changes
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
docs/operators/aws_cloudwatch_input.md Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch_persist.go Outdated Show resolved Hide resolved
operator/builtin/input/aws/cloudwatch/cloudwatch_test.go Outdated Show resolved Hide resolved
@ericwholt ericwholt requested review from djaglowski and jsirianni May 13, 2021 15:11
ericwholt and others added 3 commits May 14, 2021 12:31
@djaglowski
Copy link
Member

Log Files Logs / Second CPU Avg (%) CPU Avg Δ (%) Memory Avg (MB) Memory Avg Δ (MB)
1 1000 1.4655244 -0.08623445 128.60951 +2.229393
1 5000 5.2587914 -0.10341644 134.84766 +0.28677368
1 10000 10.327613 -0.46573067 145.13106 +8.112732
1 50000 51.293816 +0.89669037 173.93521 +1.260376
1 100000 98.398964 +1.5358505 222.18697 -15.4761505
10 100 1.9483016 -0.06904864 133.15611 +4.928604
10 500 6.0001364 -0.22432709 135.77815 -0.28515625
10 1000 12.0867 -0.08550644 144.72386 +0.94799805
10 5000 55.862877 -1.6371498 185.2015 -0.7293854
10 10000 104.811554 -4.657997 228.36139 +1.6380615

@djaglowski
Copy link
Member

Please fix tests

djaglowski
djaglowski approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work @ericwholt!

@ericwholt ericwholt merged commit a7077d1 into master May 18, 2021
@ericwholt ericwholt deleted the operator-aws-cloudwatch-logs branch May 18, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsirianni jsirianni jsirianni approved these changes

@djaglowski djaglowski djaglowski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericwholt @djaglowski @jsirianni