Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[serverless] Async support #8065

Merged

Conversation

DarcyRaynerDD
Copy link
Contributor

What does this PR do?

Supports async flushing, reducing latency of functions. Flushing will no longer block the lambda function from completing.
Also, sends 'Extension' logs by default.

Motivation

Reduce latency effect on functions.

Additional Notes

Anything else we should know when reviewing?

Describe how to test your changes

Tested manually, and added some unit tests.

@DarcyRaynerDD DarcyRaynerDD added this to the Triage milestone May 3, 2021
@DarcyRaynerDD DarcyRaynerDD self-assigned this May 3, 2021
@DarcyRaynerDD DarcyRaynerDD requested a review from a team as a code owner May 3, 2021 20:42
// and called the /hello route on the agent
clientLibReady bool

// datadogLambdaApiUsed tracks whether the layer has called the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the second part of this comment is missing


// Wait on this WaitGroup in controllers to be sure that the Daemon is ready.
// (i.e. that the DogStatsD server is properly instantiated)
ReadyWg *sync.WaitGroup

InvcWg *sync.WaitGroup
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a docs comment here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is maybe a style thing, but I find code more readable with fewer abbreviations, so I would use invocationWg

log.Debug("Switching to flush strategy:", newStrat)
d.flushStrategy = newStrat
}
if !shutdown {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the comment above this might need to be updated (I already changed it in my PR though so this could happen as part of a merge conflict resolution)

d.httpServer.Shutdown(ctx)
}

// WaitUntilClientReady will wait until the client library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing second half of comment

@@ -189,6 +198,42 @@ func (d *Daemon) EnableLogsCollection() (string, chan *logConfig.ChannelMessage,
return httpAddr, logsChan, nil
}

// StartInvocation tells the daemon the invocation began
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like these functions -- very clear and to the point!

@@ -66,8 +72,19 @@ func (d *Daemon) AutoSelectStrategy() flush.Strategy {
// flushing at least every 10 seconds (at the start of the invocation)
// TODO(remy): compute a proper interval instead of hard-coding 10 seconds
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this TODO?

@@ -276,6 +322,7 @@ func (f *Flush) ServeHTTP(w http.ResponseWriter, r *http.Request) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that the doc comment for Flush might be out of date

Copy link
Contributor

@nhinsch nhinsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few style/docs nits
Substance looks 💯 to me -- quite clear and easy to follow

pkg/serverless/interval.go Show resolved Hide resolved
pkg/serverless/protocol.go Show resolved Hide resolved
@DarcyRaynerDD DarcyRaynerDD merged commit 5957938 into release/lambda-extension-v8 May 6, 2021
@DarcyRaynerDD DarcyRaynerDD deleted the darcy.rayner/fast-invoke-response branch May 6, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants