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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[extension/ecsobserver] Add get ip and port from task #3133

Merged
merged 3 commits into from
May 5, 2021

Conversation

pingleig
Copy link
Contributor

Description:

Follow up on #2734 Add get private ip and mapped port from ecs task

Link to tracking Issue:

Previous PRs

Testing:

unit test

Documentation:

No new doc. See existing doc

@pingleig pingleig requested a review from jrcamp as a code owner April 15, 2021 17:48
@pingleig pingleig requested a review from a team April 15, 2021 17:48
@pingleig
Copy link
Contributor Author

cc @jrcamp @mxiamxia

extension/observer/ecsobserver/matcher.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/matcher.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/task.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/task.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/task.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/task.go Outdated Show resolved Hide resolved
extension/observer/ecsobserver/task.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Please make sure the build passes.

@pingleig
Copy link
Contributor Author

I think the build failed (12days ago due to dockerhub problems). I need to push new commits to address review comments so we don't need to retry the build manually (I think retry on circleci has permission requirement.)

circleci logs

Failed
    TestDefaultMetricsIntegration: container.go:109: failed pulling image docker.io/library/nginx:1.17: Error response from daemon: received unexpected HTTP status: 503 Service Unavailable

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #3133 (ae2df33) into main (17399fa) will decrease coverage by 0.28%.
The diff coverage is 100.00%.

❗ Current head ae2df33 differs from pull request most recent head 8e1538e. Consider uploading reports for the commit 8e1538e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
- Coverage   91.91%   91.63%   -0.29%     
==========================================
  Files         494      487       -7     
  Lines       23939    23574     -365     
==========================================
- Hits        22003    21601     -402     
- Misses       1429     1463      +34     
- Partials      507      510       +3     
Flag Coverage Δ
integration 63.30% <ø> (-0.39%) ⬇️
unit 90.63% <100.00%> (-0.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
extension/observer/ecsobserver/matcher.go 100.00% <100.00%> (ø)
extension/observer/ecsobserver/task.go 100.00% <100.00%> (ø)
exporter/humioexporter/config.go 94.11% <0.00%> (-5.89%) ⬇️
exporter/newrelicexporter/transformer.go 95.62% <0.00%> (-4.38%) ⬇️
exporter/signalfxexporter/config.go 83.67% <0.00%> (-2.38%) ⬇️
exporter/newrelicexporter/newrelic.go 94.23% <0.00%> (-2.01%) ⬇️
exporter/datadogexporter/metadata/ec2/ec2.go 53.33% <0.00%> (-1.99%) ⬇️
exporter/googlecloudexporter/spandata.go 83.83% <0.00%> (-1.88%) ⬇️
...r/resourcedetectionprocessor/internal/testutils.go 82.35% <0.00%> (-0.99%) ⬇️
...resourcedetectionprocessor/internal/aws/ecs/ecs.go 85.48% <0.00%> (-0.88%) ⬇️
... and 91 more

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 84952e2...8e1538e. Read the comment docs.

@pingleig
Copy link
Contributor Author

pingleig commented Apr 27, 2021

@Aneurysm9 @mxiamxia PTAL

btw: build failed for load test the extension is not enabled, so it should be loadtest's own problem.

Force pushed same commit to rerun the CI.

From steps the CPU usage is higher than expected in load test

# Test PerformanceResults
Started: Wed, 28 Apr 2021 18:19:58 +0000

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
Log10kDPS/OTLP                          |FAIL  |      3s|    41.3|    41.0|         33|         66|     17500|         17400|CPU consumption is 41.0%, max expected is 35%
Log10kDPS/filelog                       |PASS  |     15s|    16.8|    19.0|         60|         73|    150000|        150000|
Log10kDPS/filelog_checkpoints           |PASS  |     15s|    16.6|    17.3|         60|         73|    150000|        150000|

@pingleig pingleig force-pushed the ecssd-task branch 2 times, most recently from ae2df33 to 1ae299b Compare April 28, 2021 18:07
@pingleig pingleig changed the title extension: ecsobserver Add get ip and port from task [extension/ecsobserver] Add get ip and port from task Apr 28, 2021
Based on review comments from Anthony

- Add error struct for ip and port error, we can report metrics later.
- MergeTarget is wrong, add unit test to make sure it is merging new
  target only once instead of multiply by existing targets :D
- Fix typos
Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

7 participants