AutoSpotting
AutoSpotting copied to clipboard
Xray initial support
Initial implementation of #169
Issue Type
- Feature Pull Request
Summary
Added initial support for X-Ray tracing.
To be enabled it requires to enable option Enable active tracing
for Lambda. See https://docs.aws.amazon.com/xray/latest/devguide/xray-services-lambda.html for details.
Current implementation only trace calls to AWS API.
Code contribution checklist
- [x] The contribution fixes a single existing github issue, and it is linked to it.
- [x] The code is as simple as possible, readable and follows the idiomatic Go guidelines.
- [x] All new functionality is covered by automated test cases so the overall test coverage doesn't decrease.
- [x] No issues are reported when running
make full-test
. - [x] Functionality not applicable to all users should be configurable.
- [x] Configurations should be exposed through Lambda function environment variables which are also passed as parameters to the CloudFormation and Terraform stacks defined as infrastructure code.
- [x] Global configurations set from the infrastructure stack level should also support per-group overrides using tags.
- [x] Tags names and expected values should be similar to the other existing configurations.
- [x] Both global and tag-based configuration mechanisms should be tested and proven to work using log output from various test runs.
- [ ] The logs should be kept as clean as possible (use log levels as appropriate) and formatted consistently to the existing log output.
- [ ] The documentation is updated to cover the new behavior, as well as the new configuration options for both stack parameters and tag overrides.
- [ ] A code reviewer reproduced the problem and can confirm the code contribution actually resolves it.
Coverage increased (+0.02%) to 80.952% when pulling 245d7f79b4d9157a2a3a8fcf6ada8c6831f708e1 on artemnikitin:xray-initial-support into cedc8bad25a3c210ef69da9ef3c334d535519354 on cristim:master.
It's not clear how to test it properly. I tested it with uploading to my personal AWS account and running from it. There were no any EC2 instances in it, but first AWS request DescribeRegions
was traced properly. I'm assuming that all other requests will be properly traced too.
Regarding codeclimate issue. It doesn't like then function accepting 5 arguments...
Is is really needed to pass the ctx as argument to all the functions?
Can't we set it as a field in the structs and take it from there?
@cristim According to https://github.com/golang/go/wiki/CodeReviewComments#contexts it's the best practice...
Does this PR requires a review? Whether yes or no, I guess it could be great to label it based on its status
@xlr-8 I added some, but feel free to add more if you think that this is necessary.
@artemnikitin fair enough, I guess codeclimate should not count contexts in the argument list of this check
@cristim anything else from your side?
@artemnikitin tomorrow is my 20% day, I'll look at it in more depth.
I tried run this locally and got panics because of the missing context segments. I can't merge this before we ensure that the binary also works when executed locally.
$ make && ./autospotting
ok all files passed go fmt
ok all files passed go vet
GOOS=linux go build -ldflags="-X main.Version=custom-852d35b" -o autospotting
ok github.com/cristim/autospotting/core 0.010s coverage: 80.3% of statements
2018/06/29 15:10:06 Starting autospotting agent, build custom-852d35b
2018/06/29 15:10:06 Parsed command line flags: regions='' min_on_demand_number=0 min_on_demand_percentage=0.0 allowed_instance_types= disallowed_instance_types= on_demand_price_multiplier=1.00 spot_price_buffer_percentage=10.000 bidding_policy=normal tag_filters= tag_filter_mode=opt-in spot_product_description=Linux/UNIX (Amazon VPC)
2018/06/29 15:10:06 main.go:127: Scanning for available AWS regions
panic: failed to begin subsegment named 'ec2': segment cannot be found.
goroutine 1 [running]:
github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/strategy/ctxmissing.(*DefaultRuntimeErrorStrategy).ContextMissing(0x11e0af0, 0xa5fd00, 0xc4212d5d30)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/strategy/ctxmissing/default_context_missing.go:47 +0x35
github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/xray.BeginSubsegment(0xca5d80, 0xc4200ac010, 0xbf8c86, 0x3, 0x0, 0x0, 0x0)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/xray/segment.go:170 +0x661
github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/xray.glob..func1(0xc42020c800)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-xray-sdk-go/xray/aws.go:58 +0xa1
github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request.(*HandlerList).Run(0xc42020c948, 0xc42020c800)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request/handlers.go:213 +0x9d
github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request.(*Request).Build(0xc42020c800, 0xc42006fb00, 0x484376)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request/request.go:357 +0x68
github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request.(*Request).Sign(0xc42020c800, 0x60589e8, 0x11c32e0)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request/request.go:378 +0x2f
github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request.(*Request).Send(0xc42020c800, 0x0, 0x0)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/aws/request/request.go:486 +0x16d
github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/service/ec2.(*EC2).DescribeRegionsWithContext(0xc420170080, 0x7f6fef4b6678, 0xc4200ac010, 0xc420079b80, 0x0, 0x0, 0x0, 0x49, 0xc42006fc98, 0x55bc08)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/vendor/github.com/aws/aws-sdk-go/service/ec2/api.go:12169 +0xa4
github.com/cristim/autospotting/core.getRegions(0xca5d80, 0xc4200ac010, 0xcb4880, 0xc420170080, 0x30, 0xc4200b2120, 0x0, 0x0, 0x4cf570)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/core/main.go:129 +0x12a
github.com/cristim/autospotting/core.Run(0xca5d80, 0xc4200ac010, 0xc42025c270)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/core/main.go:37 +0x131
main.run(0xca5d80, 0xc4200ac010)
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/autospotting.go:62 +0x41b
main.main()
/home/magherus/devel/gocode/src/github.com/cristim/autospotting/autospotting.go:30 +0x82
FAIL: 2
Forgot to mention:
It also fails if I tried to make the ContextMissingStrategy
not panic by using this in main
, but it just panics somewhere else:
func main() {
if os.Getenv("AWS_LAMBDA_FUNCTION_NAME") != "" {
lambda.Start(run)
} else {
ctx, _ := xray.ContextWithConfig(
context.Background(),
xray.Config{ContextMissingStrategy: ctxmissing.NewDefaultLogErrorStrategy()})
run(ctx)
}
}```
@cristim I will look into that. Didn't try to run it locally...
@cristim I added change with the fix for a panic. For me locally everything works. Please check it when you will have time.
@xlr-8 will you have time to look on it?
@xlr-8 to enable it you need to check Enable active tracing
in Lambda. With our setup scripts, it's disabled by default. The same option also enables or disables X-Ray in general.
In my opinion, it makes sense to have this option configurable and disabled as it is right now.
Now log level for X-Ray configured via xray_log_level
parameter
@xlr-8 to enable it you need to check Enable active tracing in Lambda. With our setup scripts, it's disabled by default. The same option also enables or disables X-Ray in general. In my opinion, it makes sense to have this option configurable and disabled as it is right now.
Isn't it something that can be enabled via terraform / cloudformation directly? Otherwise sounds good to me!
EDIT: Also, the IAM permission of the lambda function might need to be enriched for the tracing
Isn't it something that can be enabled via terraform / cloudformation directly?
Yes, it should be possible to do with these tools. I will check it...
@artemnikitin please consider rebasing this PR on top of the recent state of the code.