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

Add --name option to 'chalice logs' command #848

Closed
wants to merge 5 commits into from

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented May 19, 2018

Issue #, if available:

#841.

Description of changes:

Adds a --name option to chalice logs command. The original logs command was written in when there was only a single lambda function. This now adds support for all lambda functions.

Example

Given this app:

from chalice import Chalice

app = Chalice(app_name='multilog')
app.debug = True

i = 0

def incr_counter():
    global i
    i += 1


@app.lambda_function()
def foo(event, context):
    incr_counter()
    app.log.debug("Invoking from function foo: %s", i)
    return {'hello': 'world'}


@app.lambda_function()
def bar(event, context):
    incr_counter()
    app.log.debug("Invoking from function bar: %s", i)
    return {'hello': 'world'}


@app.route('/')
def index():
    incr_counter()
    app.log.info("Invoking from '/' route: %s", i)
    return {'hello': 'world'}

After invoking the functions a few times:

$ http https://abcd.execute-api.us-west-2.amazonaws.com/api/
$ aws lambda invoke --function-name multilog-dev-foo /dev/stdout
$ aws lambda invoke --function-name multilog-dev-bar /dev/stdout

The default is still for the API handler lambda so there's no change in behavior:

2018-05-19 09:59:44.454000 59fc08 multilog - INFO - Invoking from '/' route: 1
2018-05-19 09:59:44.977000 59fc08 multilog - INFO - Invoking from '/' route: 2
2018-05-19 09:59:45.496000 59fc08 multilog - INFO - Invoking from '/' route: 3
2018-05-19 09:59:46.076000 59fc08 multilog - INFO - Invoking from '/' route: 4
2018-05-19 09:59:46.637000 59fc08 multilog - INFO - Invoking from '/' route: 5
2018-05-19 09:59:47.192000 59fc08 multilog - INFO - Invoking from '/' route: 6
2018-05-19 09:59:47.775000 59fc08 multilog - INFO - Invoking from '/' route: 7

But you can now retrieve logs for your other lambda functions as well:

$ chalice logs -n foo
2018-05-19 10:01:16.966000 d47fc8 multilog - DEBUG - Invoking from function foo: 1
2018-05-19 10:01:21.994000 d47fc8 multilog - DEBUG - Invoking from function foo: 2
2018-05-19 10:01:22.615000 d47fc8 multilog - DEBUG - Invoking from function foo: 3
2018-05-19 10:01:23.204000 d47fc8 multilog - DEBUG - Invoking from function foo: 4
...

$ chalice logs -n bar
2018-05-19 10:01:39.631000 577e3a multilog - DEBUG - Invoking from function bar: 1
2018-05-19 10:01:40.340000 577e3a multilog - DEBUG - Invoking from function bar: 2
2018-05-19 10:01:41.008000 577e3a multilog - DEBUG - Invoking from function bar: 3
2018-05-19 10:01:41.644000 577e3a multilog - DEBUG - Invoking from function bar: 4
2018-05-19 10:01:42.245000 577e3a multilog - DEBUG - Invoking from function bar: 5

Note that the argument of -n/--name is the logical resource name, that is foo and bar, which match the def foo(): ..., def bar(): ... in the app.py file.

@jamesls jamesls requested review from stealthycoin and kyleknap May 19, 2018 17:05
@codecov-io
Copy link

codecov-io commented May 19, 2018

Codecov Report

Merging #848 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #848      +/-   ##
==========================================
+ Coverage   94.81%   95.03%   +0.21%     
==========================================
  Files          21       21              
  Lines        3741     3743       +2     
  Branches      487      487              
==========================================
+ Hits         3547     3557      +10     
+ Misses        130      123       -7     
+ Partials       64       63       -1
Impacted Files Coverage Δ
chalice/logs.py 100% <ø> (ø) ⬆️
chalice/cli/__init__.py 81.5% <100%> (+2.71%) ⬆️
chalice/cli/factory.py 93.22% <100%> (+2.54%) ⬆️

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 f079d54...547b2f2. Read the comment docs.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks good. Just had a small comment/question. Otherwise 🚢

@@ -162,16 +162,19 @@ def delete(ctx, profile, stage):
default=False,
help='Controls whether or not lambda log messages are included.')
@click.option('--stage', default=DEFAULT_STAGE_NAME)
@click.option('-n', '--name',
help='The name of the lambda function to retrieve logs from.',
default='api_handler')
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on using chalice.constants.DEFAULT_HANDLER_NAME instead of the hard coded string here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good catch. I'll update before merging.

Copy link
Contributor

@stealthycoin stealthycoin left a comment

Choose a reason for hiding this comment

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

Might be nice to update the docs as well in logging.rst and mention that --name will refer to either the actual python function name or whatever is provided by the name kwarg.

@jamesls
Copy link
Member Author

jamesls commented May 21, 2018

Merged via 3ce3f79

@jamesls jamesls closed this May 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants