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

Introducing AWS Lambda Limits and Usage #366

Merged
merged 12 commits into from
Dec 11, 2018
Merged

Introducing AWS Lambda Limits and Usage #366

merged 12 commits into from
Dec 11, 2018

Conversation

kilsbo
Copy link
Contributor

@kilsbo kilsbo commented Nov 4, 2018

Summary

Adds support for AWS Lambda service, using this AWS API call response (aws lambda get-account-settings):

{
    "AccountLimit": {
        "CodeSizeUnzipped": 262144000, 
        "UnreservedConcurrentExecutions": 1000, 
        "ConcurrentExecutions": 1000, 
        "CodeSizeZipped": 52428800, 
        "TotalCodeSize": 80530636800
    }, 
    "AccountUsage": {
        "FunctionCount": 12, 
        "TotalCodeSize": 2167198
    }
}

Comments from the implementor

In the code base of awslimitchecker, usages and limits are treated equally. However, this is not true for AWS API in regards of the AWS Lambda service. Therefore, I've been coding it in the Lambda Service class to handle this, returning back different limits and usage metric points depending if you issue the -l or -u parameter upon execution of awslimitchecker. This may or maynot pass PR/code review, and I'm happy to oblige if anything around this is determined in need of a change.

My contribution is being made under the same license as the awslimitchecker project (or any subsequent version of that license if adopted by awslimitchecker).

Pull Request Checklist

  • ✅ All pull requests should be against the develop branch, not master.
  • ✅ All pull requests must include the Contributor License Agreement (see below).
  • ✅ Whether or not your PR includes unit tests:
    • ✅ Please make sure you have run the exact code contained in the PR locally, and it functions as desired.
    • ✅ Please make sure the TravisCI build passes or, if not, you've corrected any obvious problems identified by the tests.
  • ✅ Code should conform to the Development Guidelines:
    • ✅ pep8 compliant with some exceptions (see pytest.ini)
    • ✅ 100% test coverage with pytest (with valid tests). If you have difficulty
      writing tests for the code, that's fine, just mention that in the summary and either
      ask for assistance, or clarify that you'd like someone else to handle the tests. PRs that
      include complete test coverage will usually be merged faster.
    • ✅ Complete, correctly-formatted documentation for all classes, functions and methods.
    • ✅ documentation has been rebuilt with tox -e docs
    • ✅ Connections to the AWS services should only be made by the class's
      connect() and connect_resource() methods, inherited from
      awslimitchecker.connectable.Connectable
    • ✅ All modules should have (and use) module-level loggers.
    • Commit messages should be meaningful, and reference the Issue number
      if you're working on a GitHub issue (i.e. "issue #x - "). Please
      refrain from using the "fixes #x" notation unless you are sure that the
      the issue is fixed in that commit.
    • ✅ Git history is fully intact; please do not squash or rewrite history.

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

  • The contribution included in this request (and any subsequent revisions or versions of it)
    is being made under the same license as the awslimitchecker project (the Affero GPL v3,
    or any subsequent version of that license if adopted by awslimitchecker).
  • My contribution may perpetually be included in and distributed with awslimitchecker; submitting
    this pull request grants a perpetual, global, unlimited license for it to be used and distributed
    under the terms of awslimitchecker's license.
  • I have the legal power and rights to agree to these terms.

@codecov-io
Copy link

codecov-io commented Nov 4, 2018

Codecov Report

Merging #366 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           develop   #366   +/-   ##
======================================
  Coverage      100%   100%           
======================================
  Files           29     30    +1     
  Lines         2157   2204   +47     
  Branches       324    326    +2     
======================================
+ Hits          2157   2204   +47
Impacted Files Coverage Δ
awslimitchecker/limit.py 100% <ø> (ø) ⬆️
awslimitchecker/services/lambdafunc.py 100% <100%> (ø)

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 06c992b...bb7ab32. Read the comment docs.

@MAPontes
Copy link

MAPontes commented Nov 7, 2018

Great stuff. Looking forward to this since I too need it :) 👍

@jantman
Copy link
Owner

jantman commented Nov 18, 2018

Apologies for the delay on this. I've been pretty sick and haven't had the energy for much, but I'm on the mend and didn't want to let this sit any longer.

This was discussed a bit in #363 but I'll continue the discussion here.

I really appreciate the time you've taken to implement this, but I have a few concerns with the code as it currently stands...

First and foremost, having -u and -l return different items will break most tooling I'm aware of that either parses the output of awslimitchecker or uses the Python API. While there is the technical ability to have a Limit with an unknown usage (and this shouldn't break clients, but might), you can't have a usage without a limit. All instances of AwsLimit must be declared in the get_limits() method; the find_usage() method can add usages to existing Limits, but can't declare new ones.

In a larger scope, though, I'm a bit torn on which of these things fit into the scope of awslimitchecker. The intent of this tool is to detect when you're approaching account-level limits, and warn people who can either request a limit increase via AWS Support, or do something to fix the problem. So I'm not really sure how some of these fit into that workflow... specifically, what the use case is for usages without a limit (like FunctionCount) or limits that we can't calculate usage for (like ConcurrentExecutions).

I'm interested in feedback to this, and also how @MAPontes sees this being used.

@MAPontes
Copy link

Hi Jason, hope you are feeling better.
Your comment makes a good point. My idea was to monitor in a daily basis the highest concurrent execution of all lambdas to get a report on daily performance and load. However, after reading your comment here, makes all sense to do this by monitoring the metric on cloudwatch. This was my main point for this PR.
Thinking on the scope of awslimitchecker, it really is odd to have usages without limits and vice-versa. But maybe this could be added in the future as a reporting functionality. Just an idea :)

@kilsbo
Copy link
Contributor Author

kilsbo commented Nov 19, 2018

So, should I just hide usages when they don't have a set limit?

I mean, I would like to see the usage in terms of number of functions.

Usage of total size of functions has a limit as well so that one is fine.

@jantman
Copy link
Owner

jantman commented Dec 10, 2018

@kilsbo There's not really a way to "hide" usages.

I've been digging deeper into this today. I feel really bad that this PR has been open so long (and that my open source projects haven't really gotten much attention in far too long), so I'm committed to getting this figured out. Here's where things stand right now:

The awslimitchecker API and CLI both have current support for limits without associated Usage; this is currently in place already if your account doesn't use the resource corresponding with a given limit; i.e. if you have no load balancers, the ELB/Active load balancers Limit will report a usage of zero, but the ELB/Listeners per load balancer Limit will report an empty list for Usage, and show <unknown> in the CLI. So in that respect, we should be fine to add Limits that never report a Usage, since that's a current behavior that users already see via either the CLI or the Python API (a Limit with an empty list of usages / usage_str of "").

The idea of calculating usage for a Limit that's unlimited, on the other hand, is a bit of a gray area. awslimitchecker itself is in conflict on this... the API docs for AwsLimit.get_limit() say that it can return int or None with the latter signifying a limit that is explicitly unlimited. But the docs for the AwsLimit class constructor specify an int for default_limit. I'd need to dig quite a bit through the rest of the code to tell if None is even supported internally, let alone would be supported by third-party use cases.

One other note is that for awslimitchecker to work right with the Python API and third-party integrations, you can't define any new Limits in find_usage() - all limits must be returned by get_limits().

I'm going to cut my own branch off of this and start working on it a bit and see what I can come up with.

@jantman
Copy link
Owner

jantman commented Dec 10, 2018

Just to follow up on my previous comment re: "unlimited" limits, it appears that as of #197 which was merged over two years ago in 0.5.0, we do have support for "unlimited" limits, as Trusted Advisor returns them.

So this should have just gotten a lot simpler, and there's just a documentation issue.

@jantman
Copy link
Owner

jantman commented Dec 11, 2018

@kilsbo I've pushed a bunch of commits to this branch and am going to merge the PR once the tests pass. I did make a few changes that may break your current use of this, around naming of the limits:

  • Total Code Size (GiB) changed to Total Code Size (MiB) so that the limit and usage have the same name
  • Code Size Unzipped (MiB) to Code Size Unzipped (MiB) per Function for clarity
  • Code Size Zipped (MiB) to Code Size Zipped (MiB) per Function for clarity

those should be the only functional differences from your code.

Thank you so so much for this contribution!

@jantman jantman merged commit 3e12906 into jantman:develop Dec 11, 2018
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
…te_limits_from_api because I'm horribly pedantic
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
nadlerjessie pushed a commit to nadlerjessie/awslimitchecker that referenced this pull request Feb 16, 2019
@kilsbo
Copy link
Contributor Author

kilsbo commented Apr 3, 2019

Thank you for this, much appreciated!

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