End to end tracing support - Lambda #983

bhautikpip · 2021-08-12T22:38:19Z

See for example use case: https://github.com/garrettwegan/opentelemetry-lambda/blob/main/go/sample-apps/function/function.go

…-contrib into main

…y-go-contrib into main

…-contrib into main

…y-go-contrib into main

Co-authored-by: garrettwegan <22334769+garrettwegan@users.noreply.github.com>

bhautikpip · 2021-08-13T18:47:21Z

@Aneurysm9 @MrAlias Please take a look whenever you get a chance. This PR is derived from #882

punya · 2021-09-14T21:03:56Z

detectors/aws/lambda/README.md

+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithResource(res),
+	)
+	lambda.Start(<some lambda handler>)


Does the example need either otel.SetTracerProvider(tp) or passing tp explicitly into the Lambda handler? Otherwise the variable is unused.

Good catch, made a suggestion

punya · 2021-09-21T20:08:43Z

detectors/aws/lambda/README.md

+|`faas.name` | MyLambdaFunction 
+|`faas.version` | $LATEST
+
+Of note, `faas.id` and `cloud.account.id` are not set by the Lambda resource detector because they are not available outside a Lambda invocation. For this reason, when using the AWS Lambda Instrumentation these attributes are set as additional span attributes.


Could this be solved through lazy initialization?

Not without asking the user to add more code. The faas.id and cloud.account.id attributes are only available inside of the handler, and our solution is meant to be strictly a wrapper of the handler such that we can't actually do anything inside the wrapper. So we could ask the user to lazy-initialize the tracer provider in their handler, but that's a much worse experience.

Why would the user have to do that lazy-initialization, rather than the framework? E.g., at this point it seems like you'd have access to those invocation-scoped variables. It's true that you'd have to do separately for each possible reflective type of the handler function.

I think the problem is that we will then couple the Lambda resource detector and the Lambda instrumentation, which ought to be two separate components. The Lambda instrumentation does indeed have access to those invocation-scoped attributes, and is able to initialize the tracer provider. But the Lambda resource detector must operate only with what is available in the static environment, so if a user wants to initialize a custom tracer provider outside their handler as seen in the example it will work the same.

I agree that it's unfortunate the lambda resource detector doesn't capture these attributes, but I believe this is a limitation of Lambda itself more than any individual instrumentation, which is why the span attribute is called out in the spec and there's talks of not even marking faas.id as a resource attribute at all.

Thanks for explaining. I was commenting based on "what would an operator find most useful" vs "what is a clean way to factor the code." Given the ambiguity / ongoing discussion around faas.id, I think this code makes a reasonable choice.

willarmiros · 2021-09-24T19:47:13Z

detectors/aws/lambda/README.md

+|`faas.name` | MyLambdaFunction 
+|`faas.version` | $LATEST
+
+Of note, `faas.id` and `cloud.account.id` are not set by the Lambda resource detector because they are not available outside a Lambda invocation. For this reason, when using the AWS Lambda Instrumentation these attributes are set as additional span attributes.


Not without asking the user to add more code. The faas.id and cloud.account.id attributes are only available inside of the handler, and our solution is meant to be strictly a wrapper of the handler such that we can't actually do anything inside the wrapper. So we could ask the user to lazy-initialize the tracer provider in their handler, but that's a much worse experience.

willarmiros · 2021-09-24T19:47:41Z

detectors/aws/lambda/README.md

+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithResource(res),
+	)
+	lambda.Start(<some lambda handler>)


Suggested change

lambda.Start(<some lambda handler>)

otel.SetTracerProvider(tp)

lambda.Start(myHandler)

willarmiros · 2021-09-24T19:47:58Z

detectors/aws/lambda/README.md

+	tp := sdktrace.NewTracerProvider(
+		sdktrace.WithResource(res),
+	)
+	lambda.Start(<some lambda handler>)


Good catch, made a suggestion

codeboten

I realize this PR has been needing reviews for some time, it might be possible to get the code here merged quicker if we break up the PR.

codeboten · 2021-10-07T21:08:57Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/test/go.mod

+	go.opentelemetry.io/contrib/detectors/aws/lambda v0.22.0
+	go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-lambda-go/otellambda v0.22.0
+	go.opentelemetry.io/contrib/propagators/aws v0.22.0
+	go.opentelemetry.io/otel v1.0.0-RC2


should these be updated to 1.0.0 or 1.0.1?

codeboten · 2021-10-07T21:09:33Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/go.mod

+	github.com/aws/aws-lambda-go v1.24.0
+	github.com/stretchr/testify v1.7.0
+	go.opentelemetry.io/contrib v0.22.0
+	go.opentelemetry.io/otel v1.0.0-RC2


should these be updated to 1.0.0 or 1.0.1?

Looks like it still needs to be updated?

detectors/aws/lambda/detector.go

codeboten · 2021-10-07T21:14:36Z

detectors/aws/lambda/go.mod

+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/stretchr/testify v1.7.0
+	go.opentelemetry.io/otel v1.0.0-RC2


Please update the versions

@bhautikpip if you have a minute would you be able to update these versions to stable and rebase the PR so conflicts/CI checks are resolved?

@codeboten @willarmiros updated the version and resolved the conflicts. It should pass the CI checks.

willarmiros · 2021-10-08T18:20:02Z

I realize this PR has been needing reviews for some time, it might be possible to get the code here merged quicker if we break up the PR.

I agree and wish it were made in a more modular fashion, but as it stands there are only 2 components in this PR - the Lambda instrumentation and the Lambda resource detector. We could separate them but the detector is only 5 files, and so I'm not sure a 25 file vs. 30 file PR would be more digestible & worth the effort :( Open to other suggestions though if I'm not realizing something.

detectors/aws/lambda/detector.go

alolita · 2021-10-12T23:23:07Z

@dashpole can you please review and approve. We are blocked on this PR from being merged due to inadequate number of approver reviews. 🙏

dashpole · 2021-10-19T14:33:21Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/README.md

+| Options | Input Type  | Description | Default |
+| --- | --- | --- | --- |
+| `WithTracerProvider` | `trace.TracerProvider` | Provide a custom `TracerProvider` for creating spans. Consider using the [AWS Lambda Resource Detector][lambda-detector-url] with your tracer provider to improve tracing information. | `otel.GetTracerProvider()`
+| `WithFlusher` | `otellambda.Flusher`  | This instrumentation will call the `ForceFlush` method of its `Flusher` at the end of each invocation. Should you be using asynchronous logic (such as `sddktrace's BatchSpanProcessor`) it is very import for spans to be `ForceFlush`'ed before [Lambda freezes](https://docs.aws.amazon.com/lambda/latest/dg/runtimes-context.html) to avoid data delays. | `Flusher` with noop `ForceFlush`


Would anyone ever not want to flush at the end of an invocation?

IIRC we didn't want to force Lambda customers using async patterns to incur additional execution time from this instrumentation, but I'm not 100% sure what the reasoning behind making the default no-op was. Perhaps @Aneurysm9 remembers better.

dashpole · 2021-10-19T14:35:06Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/README.md

+
+// wrap lambda handler function
+func main() {
+	lambda.Start(otellambda.WrapHandlerFunction(HandleRequest))


Perhaps InstrumentHandler() is a better description of what this does? Or NewHandler() would match otelhttp's naming.

Not sure about NewHandler since we're not instantiating a new function handler, we are indeed just wrapping it. I think InstrumentHandler() makes sense but would like to have @bhautikpip weigh in.

yep InstrumentHandler() looks better to me.

dashpole · 2021-10-19T14:48:17Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/example/main.go

+	// init aws config
+	cfg, err := awsConfig.LoadDefaultConfig(ctx)
+	if err != nil {
+		panic("configuration error, " + err.Error())


Should we return this error instead of panicing?

dashpole · 2021-10-19T14:49:10Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/example/main.go

+	input := &s3.ListBucketsInput{}
+	result, err := s3Client.ListBuckets(ctx, input)
+	if err != nil {
+		fmt.Printf("Got an error retrieving buckets, %v", err)


Should we return the error here?

dashpole · 2021-10-19T14:49:43Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/example/main.go

+		fmt.Printf("Got an error retrieving buckets, %v", err)
+	}
+
+	fmt.Println("Buckets:")


nit: Probably better to use log.Info, rather than fmt for logging. Here and elsewhere in this file.

dashpole · 2021-10-19T14:51:11Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/example/main.go

+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, "https://api.github.com/repos/open-telemetry/opentelemetry-go/releases/latest", nil)
+	if err != nil {
+		fmt.Printf("failed to create http request, %v\n", err)


Generally, the function handling the error should log, rather than logging and returning the error. Same applies below.

Are you suggesting to remove error handling? Sorry did not understand your comment.

I'm saying you (probably?) don't need the print statement, since you are returning the error.

yeah that's fair but I think we can keep this as it probably might be useful for debugging or kind of helps understand better where the error is happening.

dashpole · 2021-10-19T14:52:49Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/example/main.go

+	detector := lambdadetector.NewResourceDetector()
+	res, err := detector.Detect(ctx)
+	if err != nil {
+		fmt.Printf("failed to detect lambda resources: %v\n", err)


should we return (ideally with log.Fatalf) here?

dashpole · 2021-10-19T14:53:17Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/go.mod

+	github.com/aws/aws-lambda-go v1.24.0
+	github.com/stretchr/testify v1.7.0
+	go.opentelemetry.io/contrib v0.22.0
+	go.opentelemetry.io/otel v1.0.0-RC2


Looks like it still needs to be updated?

dashpole · 2021-10-19T14:59:36Z

instrumentation/github.com/aws/aws-lambda-go/otellambda/lambda.go

+	ctx = i.configuration.Propagator.Extract(ctx, mc)
+
+	var span trace.Span
+	spanName := os.Getenv("AWS_LAMBDA_FUNCTION_NAME")


should this be configurable through config options, rather than env vars? Or is this guaranteed to be set in lambda?

It's guaranteed to be set: https://docs.aws.amazon.com/lambda/latest/dg/configuration-envvars.html#configuration-envvars-runtime

…y-go-contrib into tracing-support-lambda

bhautikpip · 2021-10-21T23:18:11Z

@dashpole addressed your comments! Feel free to take a look again.

willarmiros · 2021-10-22T16:27:37Z

@MrAlias @Aneurysm9 now that this is approved any chance we can get it merged?

Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>

bhautikpip and others added 30 commits February 4, 2021 16:21

added failure scenario when getting container fails

dcc01de

fix test case failure

1dd54d9

add changelog

03f6fb4

Merge branch 'main' into main

5919be5

Merge branch 'main' into main

ebd45b4

fix ecs resource detector bug

e1ff7d0

Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…

041d9b8

…-contrib into main

fix struct name as per golint suggestion

e12a4b1

fix merge conflict

e906d2a

minor changes

86c04d1

added NewResourceDetector func and interface assertions

047f5d0

fix golint failure

21db8fa

minor changes to address review comments

5204d27

Merge branch 'main' into main

c9c1bca

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

c956d4b

…y-go-contrib into main

Merge branch 'main' of https://github.com/bhautikpip/opentelemetry-go…

12c6c74

…-contrib into main

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

e042a6f

…y-go-contrib into main

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

1f6b745

…y-go-contrib into main

Implement Lambda instrumentation

76a52a1

Merge branch 'open-telemetry:main' into main

f0ec58b

Fix filepath

f048e4b

Add Lambda Dependabot checks

23638fb

Fix filepath from copy

439023e

Decouple SDK from instrumentation

f7ac5db

Improve usage of make

e932685

Improve comment clarity

8c86bb4

Minor refactoring

debba6c

Add example

d481dcb

Add license

fedb803

Add gomod checks for otellambda /example and /test modules

38a9e91

bhautikpip requested review from pellared and XSAM as code owners August 13, 2021 18:43

bhautikpip and others added 2 commits August 13, 2021 11:43

Update CHANGELOG.md

1754a23

Co-authored-by: garrettwegan <22334769+garrettwegan@users.noreply.github.com>

Merge branch 'main' into tracing-support-lambda

8445736

Aneurysm9 approved these changes Aug 13, 2021

View reviewed changes

punya reviewed Sep 21, 2021

View reviewed changes

willarmiros reviewed Sep 24, 2021

View reviewed changes

codeboten reviewed Oct 7, 2021

View reviewed changes

willarmiros reviewed Oct 8, 2021

View reviewed changes

detectors/aws/lambda/detector.go Show resolved Hide resolved

bhautikpip added 2 commits October 13, 2021 11:07

rebase and fix merge conflict with main branch

c0c6f30

updated dependencies to latest version

92b045b

NathanielRN mentioned this pull request Oct 14, 2021

Add instrumentation for AWS Lambda Service - pkg metadata files (Part 1/2) open-telemetry/opentelemetry-python-contrib#739

Merged

7 tasks

dashpole reviewed Oct 19, 2021

View reviewed changes

bhautikpip added 4 commits October 19, 2021 12:20

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

4df95c7

…y-go-contrib into tracing-support-lambda

added logging library and minor changes

93e9916

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

bd387fd

…y-go-contrib into tracing-support-lambda

modified lambda handler function name

cbc1242

dashpole approved these changes Oct 21, 2021

View reviewed changes

Aneurysm9 and others added 3 commits October 25, 2021 09:33

Merge branch 'main' into tracing-support-lambda

8ee0d8a

Update detectors/aws/lambda/detector.go

8dd0fe8

Co-authored-by: William Armiros <54150514+willarmiros@users.noreply.github.com>

Merge branch 'main' into tracing-support-lambda

fa51016

Aneurysm9 merged commit d86dc5a into open-telemetry:main Oct 25, 2021

willarmiros mentioned this pull request Oct 25, 2021

chore: wired up lambda modules for release #1375

Merged

MrAlias mentioned this pull request Oct 27, 2021

Release v1.1.0/v0.26.0 #1377

Merged

pellared added this to the untracked milestone Nov 8, 2024

	lambda.Start(<some lambda handler>)
	otel.SetTracerProvider(tp)
	lambda.Start(myHandler)

End to end tracing support - Lambda #983

End to end tracing support - Lambda #983

Conversation

bhautikpip commented Aug 12, 2021

bhautikpip commented Aug 13, 2021

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

Choose a reason for hiding this comment

willarmiros Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willarmiros commented Oct 8, 2021

alolita commented Oct 12, 2021

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

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

bhautikpip Oct 19, 2021 • edited Loading

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

bhautikpip commented Oct 21, 2021

willarmiros commented Oct 22, 2021

willarmiros Sep 24, 2021 •

edited

Loading

bhautikpip Oct 19, 2021 •

edited

Loading