AutoSpotting icon indicating copy to clipboard operation
AutoSpotting copied to clipboard

Xray initial support

Open artemnikitin opened this issue 6 years ago • 20 comments

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

  1. [x] The contribution fixes a single existing github issue, and it is linked to it.
  2. [x] The code is as simple as possible, readable and follows the idiomatic Go guidelines.
  3. [x] All new functionality is covered by automated test cases so the overall test coverage doesn't decrease.
  4. [x] No issues are reported when running make full-test.
  5. [x] Functionality not applicable to all users should be configurable.
  6. [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.
  7. [x] Global configurations set from the infrastructure stack level should also support per-group overrides using tags.
  8. [x] Tags names and expected values should be similar to the other existing configurations.
  9. [x] Both global and tag-based configuration mechanisms should be tested and proven to work using log output from various test runs.
  10. [ ] The logs should be kept as clean as possible (use log levels as appropriate) and formatted consistently to the existing log output.
  11. [ ] The documentation is updated to cover the new behavior, as well as the new configuration options for both stack parameters and tag overrides.
  12. [ ] A code reviewer reproduced the problem and can confirm the code contribution actually resolves it.

artemnikitin avatar Jun 24 '18 10:06 artemnikitin

Coverage Status

Coverage increased (+0.02%) to 80.952% when pulling 245d7f79b4d9157a2a3a8fcf6ada8c6831f708e1 on artemnikitin:xray-initial-support into cedc8bad25a3c210ef69da9ef3c334d535519354 on cristim:master.

coveralls avatar Jun 24 '18 10:06 coveralls

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.

artemnikitin avatar Jun 24 '18 10:06 artemnikitin

Regarding codeclimate issue. It doesn't like then function accepting 5 arguments...

artemnikitin avatar Jun 24 '18 10:06 artemnikitin

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 avatar Jun 25 '18 09:06 cristim

@cristim According to https://github.com/golang/go/wiki/CodeReviewComments#contexts it's the best practice...

artemnikitin avatar Jun 25 '18 13:06 artemnikitin

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 avatar Jun 25 '18 13:06 xlr-8

@xlr-8 I added some, but feel free to add more if you think that this is necessary.

artemnikitin avatar Jun 25 '18 13:06 artemnikitin

@artemnikitin fair enough, I guess codeclimate should not count contexts in the argument list of this check

cristim avatar Jun 25 '18 14:06 cristim

@cristim anything else from your side?

artemnikitin avatar Jun 28 '18 06:06 artemnikitin

@artemnikitin tomorrow is my 20% day, I'll look at it in more depth.

cristim avatar Jun 28 '18 12:06 cristim

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

cristim avatar Jun 29 '18 13:06 cristim

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 avatar Jun 29 '18 13:06 cristim

@cristim I will look into that. Didn't try to run it locally...

artemnikitin avatar Jun 29 '18 15:06 artemnikitin

@cristim I added change with the fix for a panic. For me locally everything works. Please check it when you will have time.

artemnikitin avatar Jul 01 '18 13:07 artemnikitin

@xlr-8 will you have time to look on it?

artemnikitin avatar Jul 06 '18 17:07 artemnikitin

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

artemnikitin avatar Jul 08 '18 11:07 artemnikitin

Now log level for X-Ray configured via xray_log_level parameter

artemnikitin avatar Jul 10 '18 20:07 artemnikitin

@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

xlr-8 avatar Jul 10 '18 20:07 xlr-8

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 avatar Jul 10 '18 22:07 artemnikitin

@artemnikitin please consider rebasing this PR on top of the recent state of the code.

cristim avatar Nov 29 '18 23:11 cristim