[NH-78291] Lambda support #98
Conversation
| @@ -1280,7 +1280,7 @@ func (d *DefaultDialer) Dial(p DialParams) (*grpc.ClientConn, error) { | |||
| opts = append(opts, grpc.WithContextDialer(newGRPCProxyDialer(p))) | |||
| } | |||
|
|
|||
| return grpc.Dial(p.Address, opts...) | |||
| return grpc.NewClient(p.Address, opts...) | |||
There was a problem hiding this comment.
Dial was deprecated in favor of NewClient in the upstream version of grpc
There was a problem hiding this comment.
Really appreciate the judicious comments @swi-jared! I left some questions, the main question being whether errors are captured.
| s.flags, | ||
| s.value, | ||
| s.ttl, | ||
| s.args, |
There was a problem hiding this comment.
Tangent question, the settings ttl handling is in a separate PR yes?
There was a problem hiding this comment.
We don't have TTL handling at the moment. There's a problem with the TTL being a number of seconds as opposed to a timestamp, meaning that we read/update the file and don't have an idea of when the TTL seconds should be applied from.
There was a problem hiding this comment.
Good point, I wonder if the other core libraries just calculate it as "from the most recent read". But as you mentioned, this can be a follow-on PR.
| log.Info("Stopping settings file watcher.") | ||
| exit <- true | ||
| } | ||
|
|
||
| func waitForSettingsFile() { | ||
| var timeout = 1 * time.Second |
There was a problem hiding this comment.
It looks like this multiplication is redundant?
There was a problem hiding this comment.
The time.Duration type is nanoseconds. This is the idiomatic way to set that type, with the correct value, in nanoseconds.
There was a problem hiding this comment.
Oh, I realized you meant the 1 * part is unnecessary. This is correct, though I prefer to be explicit.
There was a problem hiding this comment.
It seems like the we're doing this for the type conversion side effect of the multiplication?
| return nil, err | ||
| } | ||
| proc := processor.NewInboundMetricsSpanProcessor(registry) | ||
| prop := propagation.NewCompositeTextMapPropagator( |
There was a problem hiding this comment.
Is the list of propagators customizable outside of lambda? Maybe not for this PR if at all and I don't think it's in our internal spec.
There was a problem hiding this comment.
No, not in our SDK at least. I just copied this from Start so it's probably worth thinking about updating.
| @@ -95,8 +95,8 @@ func newSettingLambdaFromFile() (*settingLambdaNormalized, error) { | |||
| return nil, err | |||
| } | |||
| // Settings file should be an array with a single settings object | |||
| var settingLambdas []settingLambdaFromFile | |||
| if err := json.Unmarshal(settingBytes, &settingLambdas); err != nil { | |||
| var settingLambdas []*settingLambdaFromFile | |||
There was a problem hiding this comment.
Thank you for tidying my lil mess
instrumentation/github.com/aws/aws-lambda-go/swolambda/swolambda.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the explains and revisits @swi-jared, looks good from my perspective if ttl is a follow-on!
This is the last mile of work for getting Go APM to work in Lambda. NOTE: this is merging into the
lambdabranch, and we'll figure out the versioning in the instrumentation'sgo.modafter merging.