Xray initial support #273

artemnikitin · 2018-06-24T10:27:08Z

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

#169

…n/autospotting into xray-initial-support

coveralls · 2018-06-24T10:31:27Z

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

artemnikitin · 2018-06-24T10:32:08Z

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 · 2018-06-24T10:32:44Z

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

cristim · 2018-06-25T09:51:16Z

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?

artemnikitin · 2018-06-25T13:30:02Z

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

xlr-8 · 2018-06-25T13:37:13Z

Does this PR requires a review? Whether yes or no, I guess it could be great to label it based on its status

artemnikitin · 2018-06-25T13:39:32Z

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

cristim · 2018-06-25T14:36:32Z

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

artemnikitin · 2018-06-28T06:48:55Z

@cristim anything else from your side?

cristim · 2018-06-28T12:10:20Z

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

cristim · 2018-06-29T13:12:46Z

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 · 2018-06-29T13:32:20Z

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)
	}
}```

artemnikitin · 2018-06-29T15:36:50Z

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

artemnikitin · 2018-07-01T13:06:04Z

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

cristim · 2018-07-01T19:35:52Z

autospotting.go

@@ -58,7 +59,10 @@ func run(ctx context.Context) {
 		conf.TagFilteringMode,
 		conf.SpotProductDescription)

-	xray.Configure(xray.Config{LogLevel: "error"})
+	xray.Configure(xray.Config{
+		ContextMissingStrategy: ctxmissing.NewDefaultLogErrorStrategy(),


Thanks, this looks much better than I had attempted... I'm just sorry I wasted a few hours trying to fix this myself without any results.

I'll try it out tomorrow.

Worked like a charm!

cristim

I confirm the panics are fixed, but I'd leave the final verdict to @xlr-8

artemnikitin · 2018-07-06T17:39:17Z

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

xlr-8

Nice work! I wonder however if it shouldn't be made optional as X-Ray isn't free, we currently have very little view on how much it would cost depending on the setups.

Traces are pretty cheap for sure, but if one is generated at each method call, and then runs of lambda every 2min or so I wouldn't like to leave users with a significant cost increase.

On top of that, X-Ray is mostly for the profiling of the application. So it might be worth enabling it by default on all installs.

WDYS?

xlr-8 · 2018-07-06T11:27:29Z

autospotting.go

+	xray.Configure(xray.Config{
+		ContextMissingStrategy: ctxmissing.NewDefaultLogErrorStrategy(),
+		LogLevel:               "error",
+	})


Can this be configurable from an env variable?

What exactly do you want to configure in this place? Log level?

I made log level configurable via flag, but it looks like there is a bug in X-Ray SDK. aws/aws-xray-sdk-go#60. At the moment, it will post a particular error message ignoring log level...

xlr-8 · 2018-07-07T10:10:28Z

core/region.go

@@ -79,25 +81,25 @@ func (r *region) processRegion() {
 	r.setupAsgFilters()

 	logger.Println("Scanning for enabled AutoScaling groups in ", r.name)
-	r.scanForEnabledAutoScalingGroups()
+	r.scanForEnabledAutoScalingGroups(context.Background())


The context.Background is usually used for empty context (test/main), so I'm a bit surprised to see it invoked various times. What's the reason behind this?

Hmm... context.Background() should be used in tests... I will check, there shouldn't be such things in code

Thanks for catching

artemnikitin · 2018-07-08T11:55:20Z

@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 · 2018-07-10T20:43:29Z

Now log level for X-Ray configured via xray_log_level parameter

xlr-8 · 2018-07-10T20:53:47Z

@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

artemnikitin · 2018-07-10T22:18:07Z

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

cristim · 2018-11-29T23:03:30Z

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

artemnikitin added 3 commits June 24, 2018 11:44

Initial support of X-Ray tracing

276782e

#169

Initial support of X-Ray tracing

27e1d98

Merge branch 'xray-initial-support' of https://github.com/artemnikiti…

852d35b

…n/autospotting into xray-initial-support

artemnikitin requested review from xlr-8 and cristim June 24, 2018 10:28

artemnikitin added Type: Enhancement Importance: Nice to have labels Jun 25, 2018

Fix panic on missing context

1ea38d8

cristim reviewed Jul 1, 2018

View reviewed changes

cristim approved these changes Jul 2, 2018

View reviewed changes

Merge branch 'master' into xray-initial-support

4367ddd

xlr-8 reviewed Jul 8, 2018

View reviewed changes

artemnikitin added 2 commits July 10, 2018 21:12

Fixing passing wrong context

ed2fa69

Configure X-Ray SDK log level via flag

245d7f7

xlr-8 added the Status: In progress label Jul 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xray initial support #273

Xray initial support #273

artemnikitin commented Jun 24, 2018 •

edited

Loading

coveralls commented Jun 24, 2018 •

edited

Loading

artemnikitin commented Jun 24, 2018

artemnikitin commented Jun 24, 2018

cristim commented Jun 25, 2018

artemnikitin commented Jun 25, 2018

xlr-8 commented Jun 25, 2018

artemnikitin commented Jun 25, 2018

cristim commented Jun 25, 2018

artemnikitin commented Jun 28, 2018

cristim commented Jun 28, 2018

cristim commented Jun 29, 2018

cristim commented Jun 29, 2018

artemnikitin commented Jun 29, 2018

artemnikitin commented Jul 1, 2018

cristim Jul 1, 2018

cristim Jul 2, 2018

cristim left a comment

artemnikitin commented Jul 6, 2018

xlr-8 left a comment

xlr-8 Jul 6, 2018

artemnikitin Jul 8, 2018

artemnikitin Jul 10, 2018

xlr-8 Jul 7, 2018

artemnikitin Jul 8, 2018

artemnikitin Jul 8, 2018

artemnikitin commented Jul 8, 2018 •

edited

Loading

artemnikitin commented Jul 10, 2018

xlr-8 commented Jul 10, 2018 •

edited

Loading

artemnikitin commented Jul 10, 2018

cristim commented Nov 29, 2018

Xray initial support #273

Are you sure you want to change the base?

Xray initial support #273

Conversation

artemnikitin commented Jun 24, 2018 • edited Loading

Issue Type

Summary

Code contribution checklist

coveralls commented Jun 24, 2018 • edited Loading

artemnikitin commented Jun 24, 2018

artemnikitin commented Jun 24, 2018

cristim commented Jun 25, 2018

artemnikitin commented Jun 25, 2018

xlr-8 commented Jun 25, 2018

artemnikitin commented Jun 25, 2018

cristim commented Jun 25, 2018

artemnikitin commented Jun 28, 2018

cristim commented Jun 28, 2018

cristim commented Jun 29, 2018

cristim commented Jun 29, 2018

artemnikitin commented Jun 29, 2018

artemnikitin commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristim left a comment

Choose a reason for hiding this comment

artemnikitin commented Jul 6, 2018

xlr-8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artemnikitin commented Jul 8, 2018 • edited Loading

artemnikitin commented Jul 10, 2018

xlr-8 commented Jul 10, 2018 • edited Loading

artemnikitin commented Jul 10, 2018

cristim commented Nov 29, 2018

artemnikitin commented Jun 24, 2018 •

edited

Loading

coveralls commented Jun 24, 2018 •

edited

Loading

artemnikitin commented Jul 8, 2018 •

edited

Loading

xlr-8 commented Jul 10, 2018 •

edited

Loading