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

fix(AWS Invocation): Ensure proper log formatting for invoke #11189

Merged
merged 2 commits into from Jun 29, 2022

Conversation

pgrzesik
Copy link
Contributor

Closes: #11188

@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #11189 (161fd48) into main (c611ce4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main   #11189   +/-   ##
=======================================
  Coverage   86.01%   86.01%           
=======================================
  Files         315      315           
  Lines       13273    13275    +2     
=======================================
+ Hits        11417    11419    +2     
  Misses       1856     1856           
Impacted Files Coverage Δ
lib/plugins/aws/invoke.js 98.27% <100.00%> (+0.06%) ⬆️
lib/plugins/aws/logs.js 86.20% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c611ce4...161fd48. Read the comment docs.

@pgrzesik pgrzesik requested a review from medikoo June 28, 2022 18:19
@@ -113,6 +113,14 @@ class AwsInvoke {
);
const logResult = Buffer.from(invocationReply.LogResult, 'base64').toString();
logResult.split('\n').forEach((line) => {
if (
// TODO: Switch to .startsWith after otel extension is improved
line.includes('⚡.') ||
Copy link
Contributor

Choose a reason for hiding this comment

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

It's long time since we do not issue such logs with extension, so I think it can safely be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -13,7 +13,7 @@ chai.use(require('sinon-chai'));

const expect = chai.expect;

describe('test/unit/lib/plugins/aws/invoke.test.js', () => {
describe.only('test/unit/lib/plugins/aws/invoke.test.js', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not commit this change (ideally it should be picked by linter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ouch, good catch

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

👍

@pgrzesik pgrzesik merged commit 01d9ad5 into main Jun 29, 2022
@pgrzesik pgrzesik deleted the filter-out-logs-in-invoke branch June 29, 2022 16:01
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.

Duplicate "END/REPORT" line in sls logs
2 participants