-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added Elastic Beanstalk Resource detector #1585
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1585 +/- ##
==========================================
- Coverage 89.24% 89.24% -0.01%
==========================================
Files 354 356 +2
Lines 17417 17441 +24
==========================================
+ Hits 15544 15565 +21
- Misses 1395 1399 +4
+ Partials 478 477 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
## Configuration | ||
|
||
```yaml | ||
# a list of resource detectors to run, valid options are: "env", "system", "gce", "ec2", "ecs" | ||
# a list of resource detectors to run, valid options are: "env", "system", "gce", "ec2", "ecs", "elasticBeanstalk" |
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.
nit: for component identifiers (e.g. processor/k8s_tagger) we do underscores instead of camel case so maybe better to do elastic_beanstalk
?
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.
Ok, I will rename
|
||
ebmd := &EbMetaData{} | ||
err = json.NewDecoder(conf).Decode(ebmd) | ||
defer conf.Close() |
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.
Would either put the defer right after the err checking or since it looks like you're done reading the file here just conf.Close()
with no defer?
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.
Good catch don't think a defer is necessary, this was just a bit too mindless of a copy-paste from an online example.
} | ||
|
||
if err != nil { | ||
log.Println("Unable to find Elastic Beanstalk config file, skipping resource detection") |
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 be using log package but the zap logger passed into the component (looks like ecs is using log too which it shouldn't have). We're not passing log instances into detectors right now so will require some refactoring to add it.
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.
Yeah I noticed there's no Zap object passed in which is why I fell back to log
, but in retrospect I probably should have just done nothing. I can remove the logging statements from this PR, and take a look at the refactor to pass in the zap logger in a separate PR.
Description:
Enhancement - adds new resource detector for AWS Elastic Beanstalk environments. It reads a file provided on all EB instances with X-Ray enabled and records the metadata it contains.
Testing:
Tested on live app and added unit tests.
Documentation:
Updated docs to match other resource detectors.