Skip to content

Added metric reporting functions #898

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

Merged
merged 28 commits into from
Apr 20, 2022

Conversation

TwistedTwigleg
Copy link
Contributor

Description of changes:

Adds metric report functions that are platform specific that can be used in the Device Defender custom metrics. Needed for the following PR: aws/aws-iot-device-sdk-cpp-v2#401


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

I wouldn't make these "custom_metric" I'd just make them top-level functions, likely adding to either process.* or system_info.*

Also, restricted to posix-only, does the implementation still need to check defines? Does it work on a mac?

@TwistedTwigleg
Copy link
Contributor Author

I wouldn't make these "custom_metric" I'd just make them top-level functions, likely adding to either process.* or system_info.*

Will do 👍

Also, restricted to posix-only, does the implementation still need to check defines? Does it work on a mac?

Unfortunately, as sysinfo.h isn't available on Mac. There is a similar function that provides some of the information used, but since Device Defender doesn't support Mac I haven't added the support for it.

@lgtm-com

This comment was marked as outdated.

@lgtm-com

This comment was marked as outdated.

Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

I apologize in advance because I've given you some red herring advice. Looking at this again, I realize that we need to hide the details here because they're very specific to linux. As a bonus in doing so (via platform-specific opaque implementations), you can also hide the state you need to make the before-after calculation to get the usage in the most recent elapsed time interval.

So I would instead make a polymorphic (per-platform) CPU sampler type (new/destroy, shouldn't need ref counting) and then you can embed the previous samples in the per-platform impl data (and have new() return NULL for Macs and Windows for now).

Downstream, device defender just creates one of these objects and calls get_cpu_usage(double *) on it at whatever rate it would like.

This only pertain to aws_get_cpu_usage. the memory and process check APIs are fine.

@lgtm-com
Copy link

lgtm-com bot commented Apr 13, 2022

This pull request introduces 1 alert when merging 3413c4e into fb1fcb6 - view on LGTM.com

new alerts:

  • 1 for Constant return type

Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

You'll find older things that don't follow this pattern, but currently new features tend to use the following pattern:

(all structures are private, either in a source file or a private header)

struct aws_blah {
   /* data used by all subclasses */

   /* pointer to vtable */

   void *impl; /* points to the start of the subclass */
};

struct aws_blah_subclass {
   struct aws_allocator *allocator;

   struct aws_blah base;

   /* subclass specific data */
};

your public operations, other than the constructors, defer to the vtable. This includes destroy.

Your constructor functions (aws_blah_subclass_new()) return a pointer to the aws_blah inside the allocated aws_blah_subclass structure.

Your vtable-referenced subclass functions (which can be static and hidden) always convert the aws_blah that they are passed into an aws_blah_subclass by casting aws_blah->impl.

I'll send you a link to a recent example (that I can't put in a public comment) shortly. It uses acquire/release rather than destroy, but other than that the pattern is the same.

@TwistedTwigleg
Copy link
Contributor Author

Thanks for the reviews! Merging into main...

@TwistedTwigleg TwistedTwigleg merged commit 9b2a8b7 into main Apr 20, 2022
@TwistedTwigleg TwistedTwigleg deleted the DeviceDefenderCustomMetricsSupport branch April 20, 2022 15:08
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.

3 participants