Moving towards composition rather than inheritance for greater modularity

[Dicee]

Feature scope

Everything

Description

I'm proposing to expose all immutable properties, as well as methods creating new objects (like widgets) without side-effect on the construct, as public rather than protected (for example, exposing LambdaFunctionMonitoring#faultRateMetric).

Use Case

One can extend subclasses of Monitoring (e.g. LambdaFunctionMonitoring) in order to override the widgets, kind of alarms created etc when the construct doesn't do what the user wants by default. This pattern works fairly well for building a construct that can be reused for several use cases. However, it doesn’t work as well for highly customized monitoring.

For example, the lambda monitoring construct really doesn’t cut it for me. Its dashboard is much too detailed for my simple Lambda and most metrics don’t make sense for me, plus I want to add some of my own, and they only apply to a single Lamba in my application, not any other. Just to show how inappropriate the default behaviour is to my use-case:

• TPS widget: my lambda gets consistently called at 1 call per minute via a CloudWatch event, so I don't care about TPS
• Latency widget: the lambda is asynchronous and will typically execute in seconds, I don't care about latency
• Error rate widget: useful, but not as a rate. Since the lambda executes so rarely, I just want to know if each run was successful or not, I don't care about the rate over a period of time. It has to succeed at every run to be healthy.
• Throttles and provisioned concurrency: don't care, I don't have any provisioning and it can't throttle as it's called once a minute (unless my latency exceeds a minute with reserved concurrency of 1 but it's impossible because my timeout is 30s)
• Invocations: don't care, I know it gets called once a minute
• Iterator: not relevant to the way the lambda is triggered
• Errors: finally a relevant widget!
• Business metrics: I need to add them myself

This is where inheritance and template classes typically fail: they’re too limited, because they impose a template. The more abstract the template, the more customizations are possible, but also the more the abstract class becomes convoluted and hard to make sense of, particularly after several levels of inheritance.

To fix these shortcomings, I propose moving towards composition rather than inheritance. Imagine code like this:

public monitorSnapshotGenerationFunction(fn: SecureFunction) {
    const period = SNAPSHOT_POLLING_RATE;
    const lambdaMonitoring = new ContraCogsLambdaMonitoring(this.monitoring, this.serviceHealthAlarmCollector, {
        lambdaFunction: fn,
        addFaultCountAlarm: {
            '': {maxErrorCount: 0 /* exclusive */, evaluationPeriods: 3, datapointsToAlarm: 3, period}
        }
    })

    this.monitoring.addSegment(new class {
        widgets() {
            return [
                lambdaMonitoring.createTitleWidget(),
                lambdaMonitoring.createErrorCountWidget(DEFAULT_WIDGET_WIDTH, DEFAULT_WIDGET_HEIGHT),
                createNewSnapshotGeneratedWidget(period), // custom business metric
            ]
        }

        alarmWidgets() {return []}
        summaryWidgets() {return []}
    })
}

This is actual code I have written using a subclass of LambdaMonitoring that exposes its properties and methods. Now I can leverage the code encapsulated in these methods/properties, while having absolute freedom on how to compose them. Pick what's useful, and leave the rest alone. The above is exactly what I need, no more, no less.

For this reason, I would make all these properties and methods public. Note this doesn’t breach anything in terms of encapsulation, the service-specific knowledge stays in the service-specific constructs and no internal mutable state gets exposed, but this allows the user to compose the perfect monitoring for their use case(s) easily.

Other information

No response

[echeung-amzn]

The rigid dashboarding has definitely been brought up in the past (#66), so this could be a potential solution for that. The actual metric/alarm functionality should theoretically be easy to add to the library as needed, but I'd be interested in hearing counterpoints (perhaps some metrics/alarms don't seem appropriate to add to the library for broader use?).

On the other hand, we're also hesitant to open up everything and have to commit to a broad API contract (#125) since it'd make some changes difficult without being forced into major version bumps + migrations for users. That said, we don't have a super concrete idea of what that'd necessarily look like in practice, so it may not be as bad as we think.

Not sure if other maintainers have thoughts around this (@voho, @ayush987goyal).

[Dicee]

I understand the argument of broadening the contract, it's a good point. That said, I think as long as we're exposing basic stuff that's unlikely to change (AWS would not revamp their metric names drastically every month), the risk is low.

Also, I just want to say that protected visibility also makes these methods and properties part of the API since many people have already extended these classes to override certain methods like the list of widgets, and in doing so they have tied themselves with some of the protected members. In my case, I literally subclassed LambdaMonitoring to expose all its protected methods and properties.

In that sense, moving from protected to public is only a design decision at this point, and will not increase the API surface already available to customers. Furthermore, the current approach also means that everybody who uses the constructs out-of-the-box is tying themselves to the specific choices of one API version, and could see all their dashboards affected by every change of the API. You can see this as a good or a bad thing, but I would personally not enjoy having my dashboards changed without it being an explicit decision from the team.

[echeung-amzn]

I just want to say that protected visibility also makes these methods and properties part of the API since many people have already extended these classes to override certain methods like the list of widgets, and in doing so they have tied themselves with some of the protected members

Indeed, I commented on that in #122 (comment):

Regarding the breaking change argument, I was actually thinking of whether we should explicitly define what we consider to be part of our "public API" that the semantic versioning revolves around. It'd seem appropriate to ensure the versioning reflects what's available as the top-level facade API, but anything more "internal", despite being exported and extensible as an escape hatch, may have looser guarantees.

Going all-in and committing to an API could be a worthwhile move at this point.

AWS would not revamp their metric names drastically every month

I'm actually more worried about the library's API than the underlying AWS metrics/constructs that we're dependant on changing.

I'd be curious to see what your subclassed more open class(es) look like.

[Dicee]

Fair fair, I'll wait for maintainers to respond, I'm interested to hear thoughts about it. Here's the subclass I created (modulo a minor customization that I removed from my original code for this gist): https://gist.github.com/Dicee/f7d5a4cc1258359a9f66f16562afb3ea

[Dicee]

Some other opinions on this request?

[djdawson3]

I would like to +1 this request. I also brought up something similar but not the same on issue # 66 as well: #66

[niebloomj]

I'd like to vote in support as well as offer a helping hand in coding. This would seriously help me out in places I am using this library.

[Dicee]

Thanks for the upvotes guys, actually if you look carefully at the thread I have sent a pull request waiting for review. I don't know how long it typically takes...

[niebloomj]

Looks like it requires a rebase which is what is blocking it. Looking forward to it! Very exciting!

[Dicee]

The PR is now pushed, so I will try to find time for the follow-up Eugene suggested (adding a test looking like the example I provided above). I want to think about it before fully committing to doing it, because for now I don't know if it would make sense to do it for just one class, and doing it for all classes might not be worth the effort.