-
Notifications
You must be signed in to change notification settings - Fork 54
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
[lambda][flare] Send minimal Lambda config data to Datadog #924
[lambda][flare] Send minimal Lambda config data to Datadog #924
Conversation
Datadog ReportBranch report: ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments
Rename classes and variables in flare-command-validator.ts for more readability
- Validate caseId and email - Add tests
…dows, Mac, and Linux
…aFunctionConfig`, not other functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I lack the context for reviewing the lambda bit but do we need jszip
? Have we tried using zlib
?
|
||
const {version} = require('../../../package.json') | ||
|
||
const ENDPOINT_URL = 'https://datad0g.com/api/ui/support/serverless/flare' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to hardcode a staging URL like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @juan-fernandez, we will hard code it for staging in the meantime. We will not merge this to the main
branch until we have the right endpoint merged in production.
We will bring another PR to update this to the user specific site endpoint in the future, wdyt?
const data = fs.readFileSync(filePath, 'utf8') | ||
const zip = new JSZip() | ||
zip.file(FUNCTION_CONFIG_FILE_NAME, data) | ||
const content = await zip.generateAsync({type: 'nodebuffer'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couldn't we use zlib
that is native? We wouldn't need a new dependency then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I will look into using zlib
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @juan-fernandez
Looks like zlib
only supports zipping single files. While this would work for this PR, we do plan on zipping multiple files in the near future, so we would eventually need a new dependency like jszip
.
What do you think?
Edit: I meant jszip
not fszip
!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nhulston do you mean jszip
?
@juan-fernandez I'd normally go for the zlib
approach too, when I researched on this, it seemed that we needed more packages to compress folders (tar
and fstream
). By using zlib
and its dependencies for our use case, previously mentioned, we would be using 550~600 kB less than if we used jszip
.
It seems that change of implementation might be trivial. But of course, this could be refactored in the future if package size is a problem. This would also not be merged directly into main since our working branch serverless-flare
is still a WIP before its first release.
What do you think @juan-fernandez?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first PR!
)" This reverts commit 174bccd.
* Create and validate flare flags * Write config to file and zip it * - Send zip to Datadog when not in dry run * Change zip method to use the `jszip` package for compatability on Windows, Mac, and Linux * Add `jszip` copyright * - Move constant endpoints - Remove redundant try/catch on `fs.createReadStream` since the higher-level function `execute` handles the error - Remove redundant `.then()` since we already use `await` * Get CLI version programmatically and set as `operator_version` * Rename `operator_version` to `datadog_ci_version` * Code review refactoring * Create thorough tests
What and why?
This PR adds functionality to the
datadog-ci lambda flare
command to automatically collect useful information from customers to help Datadog support resolve customer issues. This PR only adds functionality for getting and sending AWS Lambda function configuration for a single Lambda function/region to the support staff.Future PRs aim to:
How?
An example command would be
lambda flare -f function-name -r us-east-1 -c 1234567 -e [email protected]
This PR allows sending function configuration to the support staff through several steps:
--dry, --function, --region, --case-id, --email
(and their shorthands-d, -f, -r, -c, -e
). The--dry
flag enables a mode where theconfig.json
is printed but not saved as a file or sent.getAWSCredentials()
orrequestAWSCredentials()
functions:datadog-ci/src/commands/lambda/functions/commons.ts
Lines 234 to 249 in 08ea2aa
getLambdaFunctionConfig()
:datadog-ci/src/commands/lambda/functions/commons.ts
Lines 364 to 386 in 08ea2aa
.datadog-ci/config.json
using thefs
package.datadog-ci
folder using thejszip
package. Future PRs will save multiple files, sojszip
is required to zip an entire directoryThese changes provide a robust foundation for future PRs, ensuring efficient implementation of additional features.
Review checklist