Lambda instrumentation enhancements #1390
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bhautikpip
requested review from
Aneurysm9,
dashpole,
evantorrie and
jmacd
as code owners
bhautikpip
requested review from
MadVikingGod,
MrAlias,
paivagustavo,
pellared and
XSAM
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1390 +/- ##
=======================================
- Coverage 69.3% 69.0% -0.3%
=======================================
Files 127 127
Lines 5496 5498 +2
=======================================
- Hits 3810 3798 -12
- Misses 1538 1552 +14
Partials 148 148
|
Aneurysm9
requested changes
Nov 5, 2021
Comment on lines
30
to
39
| type MyEvent struct { | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
| func HandleRequest(ctx context.Context, name MyEvent) (string, error) { | ||
| return fmt.Sprintf("Hello %s!", name.Name ), nil | ||
| func HandleRequest(ctx context.Context) (error) { | ||
| fmt.Println("Hello World!" ) | ||
| return nil | ||
| } | ||
|
|
||
| func main() { | ||
| lambda.Start(HandleRequest) | ||
| ctx := context.Background() | ||
| lambda.Start(otellambda.InstrumentHandler(HandleRequest(ctx))) |
There was a problem hiding this comment.
I don't think this change is correct. The previous example was correct and this change makes it likely to not even compile since it reduces to:
lambda.Start(otellambda.InstrumentHandler(nil))Also, the prior state was trying to illustrate what a starting point for an uninstrumented function would look like.
| } | ||
| ``` | ||
|
|
||
| Now use the provided wrapper to instrument your basic Lambda function: | ||
| Now configure the instrumentation with the provided options to export traces to AWS X-Ray via [the OpenTelemetry Collector](https://github.com/open-telemetry/opentelemetry-collector) running as a Lambda Extension. Instructions for running the OTel Collector as a Lambda Extension can be found in the [AWS OpenTelemetry Documentation](https://aws-otel.github.io/docs/getting-started/lambda). |
There was a problem hiding this comment.
Do not conflate the otellambda instrumentation and the xrayconfig setup packages. This is documentation for otellambda and should not need to mention X-Ray at all.
| @@ -60,45 +87,6 @@ func main() { | |||
| | `WithEventToCarrier` | `func(eventJSON []byte) propagation.TextMapCarrier{}` | Function for providing custom logic to support retrieving trace header from different event types that are handled by AWS Lambda (e.g., SQS, CloudWatch, Kinesis, API Gateway) and returning them in a `propagation.TextMapCarrier` which a Propagator can use to extract the trace header into the context. | Function which returns an empty `TextMapCarrier` - new spans will be part of a new Trace and have no parent past Lambda instrumentation span | |||
| | `WithPropagator` | `propagation.Propagator` | The `Propagator` the instrumentation will use to extract trace information into the context. | `otel.GetTextMapPropagator()` | | |||
|
|
|||
| ### Usage With Options Example | |||
There was a problem hiding this comment.
Restore this section, it is a useful generic example of how to use the options this package provides.
Comment on lines
+96
to
+109
| type asyncSafeFlusher struct { | ||
| flusher Flusher | ||
| } | ||
|
|
||
| func (f asyncSafeFlusher) ForceFlush(ctx context.Context) error { | ||
| // yield processor to attempt to ensure all spans have | ||
| // been consumed and are ready to be flushed | ||
| // - see https://github.com/open-telemetry/opentelemetry-go/issues/2080 | ||
| // to be removed upon resolution of above issue | ||
| runtime.Gosched() | ||
|
|
||
| return f.flusher.ForceFlush(ctx) | ||
| } | ||
|
|
There was a problem hiding this comment.
I have just landed open-telemetry/opentelemetry-go#2335 which will enable us to remove this entirely and simply call ForceFlush on the provided TracerProvider. The entire Flusher type and its implementations and configuration options can be removed.
| @@ -11,52 +11,14 @@ This module provides recommended configuration options for [`AWS Lambda Instrume | |||
| go get -u go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-lambda-go/otellambda/xrayconfig | |||
| ``` | |||
|
|
|||
| ## Usage | |||
There was a problem hiding this comment.
Restore this section. It is distinct from the otellambda documentation.
Comment on lines
+37
to
+39
| // PrepareTracerProvider returns a TracerProvider configured with exporter, | ||
| // id generator and lambda resource detector to send trace data to AWS X-Ray via Collector | ||
| func PrepareTracerProvider(ctx context.Context) (*sdktrace.TracerProvider, error) { |
There was a problem hiding this comment.
Suggested change
| // PrepareTracerProvider returns a TracerProvider configured with exporter, | |
| // id generator and lambda resource detector to send trace data to AWS X-Ray via Collector | |
| func PrepareTracerProvider(ctx context.Context) (*sdktrace.TracerProvider, error) { | |
| // NewTracerProvider returns a TracerProvider configured with exporter, | |
| // id generator and lambda resource detector to send trace data to AWS X-Ray via Collector | |
| func NewTracerProvider(ctx context.Context) (*sdktrace.TracerProvider, error) { |
New*() is idiiomatic.
| // PrepareTracerProvider returns a TracerProvider configured with exporter, | ||
| // id generator and lambda resource detector to send trace data to AWS X-Ray via Collector | ||
| func PrepareTracerProvider(ctx context.Context) (*sdktrace.TracerProvider, error) { | ||
| log.Println("creating trace exporter") |
There was a problem hiding this comment.
Suggested change
| log.Println("creating trace exporter") |
Comment on lines
+64
to
+65
| // tracerProviderAndFlusher is not exported because it should not be used | ||
| // without the provided EventToCarrier function and XRay Propagator |
There was a problem hiding this comment.
Why should these two options not be used without the other two options?
| return otellambda.WithPropagator(xray.Propagator{}) | ||
| } | ||
|
|
||
| // AllRecommendedOptions returns a list of all otellambda.Option(s) | ||
| // recommended for the otellambda package when using AWS XRay | ||
| func AllRecommendedOptions() []otellambda.Option { | ||
| options, err := tracerProviderAndFlusher() | ||
| func AllRecommendedOptions(tp *sdktrace.TracerProvider) []otellambda.Option { |
There was a problem hiding this comment.
Suggested change
| func AllRecommendedOptions(tp *sdktrace.TracerProvider) []otellambda.Option { | |
| func RecommendedOptions(tp *sdktrace.TracerProvider) []otellambda.Option { |
The All feels superfluous. Can we drop it?
|
Closing this PR due to some local git conflicts will open a new one with the changes suggested. |
plantfansam
referenced
this pull request
in plantfansam/opentelemetry-go-contrib
Mar 18, 2022
Fix typo Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
xrayconfigpackage