Maintenance: Metrics types and Implementation

[niko-achilles]

Summary

High level overview , can be described with details in a time point.

1. Tactics to handle Data Model, that is outside the boundaries of Powertools:

• e.g: EMF Data Model that is used in Metrics.

2. serialize metrics code does additional things besides serialization, it produces as result the state of the EMF Data Model.

• e.g filters , maps , validates and then serializes : result state of the EMF Data Model

3. Concepts of types that are used for StoredMetric in Metrics

• e.g: comparison (old, new Metric), definition of, units of metric

4. Metrics Interface that is implemented by Metrics does not provide hints to a contributor when
   contributor has the intention to extend the functionality of Metrics

• e.g: when extending the interface of a given a method by introducing a new parameter the class that implements the interface does not provide feedback about the new parameter introduced in the method interface.

5. Comments of code in areas of Metrics lack of consistency and a contributor cannot derive a pattern to write comments in code when the intention is to add new functionality with comments in code .

6. Unit Tests of Metrics lack of utilities that can be reused when the intention is to introduce a new test when extending functionality of Metrics

• e.g : prepare phase , create a metric in a namespace , with dimensions, etc. is repeated in tests .

Why is this needed?

High level description of maintenance improvements

1. Tactics to handle Data Model, that is outside the boundaries of Powertools:

• e.g: EMF Data Model that is used in Metrics.

• Why is needed ? purpose:
  It will improve readability and learning for contribution when the EMF Schema as typescript interface or type is provided.
  It will also protect from type/interface explosion , given a contributor can create similar types in the types/Metrics.ts .

• Use case :
  By introducing the high-resolution metrics implementation a new type is introduced in types/Metrics.ts which is imported in Metrics.ts implementation .

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/types/Metrics.ts#L71-L75

This MetricDefinition type which is in the types/Metrics.ts is referenced in the EmfOutput type

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/types/Metrics.ts#L17-L27

This is a decision made by understanding a pattern during the implementation. The MetricUnits were referenced in the EmfOutput type, and as result the MetricDefinition type is created .
However the MetricDefinition type has the same meaning as MetricDefinition of EMF Data Model .

The same can happen by combing other properties of Emf Data Model and introduce new type aliases with the same meaning as Emf . That will mean type explosion .

Also a contributor by reading the types/Metrics.ts expects to read about types that support the mechanics of Metrics.ts .
Is the MetricDefinition a type imported in Metrics.ts used in public methods ? Does the MetricDefinition type belong to the types/Metrics.ts ?
Is the mutable EmfOutput type used in Metrics.ts ? As is defined today it is mutable or does the Output mean readonly ? . Does the EmfOutput type belong to the types/Metrics.ts ?

2. serialize metrics code does additional things besides serialization, it produces as result the state of the EMF Data Model.

• e.g filters , maps , validates and then serializes : result state of the EMF Data Model
• why is needed: readability reasons.

The index Type

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/types/Metrics.ts#L67-L69

And the mapReducer

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/Metrics.ts#L370

And the return Type EmfOutput that is produced from serialize can be improved for readability and consistency.

3. Concepts of types that are used for StoredMetrics in Metrics

• e.g: comparison (old, new Metric), definition of, units of metric
• Why is needed ? purpose: The isNew Metric is implemented as below . The if statement comparison (not equal) is used for old metrics.
  However the units are implemented with structural typing support (enum and type combination). Can a unit of an old metric be inconsistent ?

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/Metrics.ts#L477-L480

4. Metrics Interface that is implemented by Metrics does not provide hints to a contributor when
   contributor has the intention to extend the functionality of Metrics

• e.g: when extending the interface of a given a method by introducing a new parameter the class that implements the interface does not provide feedback about the new parameter introduced in the method interface.
• Why is needed ? purpose: guidance to extend the functionality with tight feedback loop when righting tests firsts and then in steps extending the functionality.

5. Comments of code in areas of Metrics lack of consistency and a contributor cannot derive a pattern to write comments in code when the intention is to add new functionality with comments in code .

• Why is needed ? purpose: which guide can be used by the contributor ? tsdoc, jsdoc or markdown that can result in a balance for rendering comments of code in IDE and API docs .
• in vs code some tsdoc or jsdoc attributes are not parsed nice , like {@link }
• should a contributor create comments in code for types ? should the methods that use the types link to the type and be clickable ?
• should the optional parameters have a code comment about the default value ?

6. Unit Tests of Metrics lack of utilities that can be reused when the intention is to introduce a new test when extending functionality of Metrics

• e.g : prepare phase of unit tests, create a new metric, add a metric , set dimensions, etc. is repeated in tests .
• Why is needed ? purpose: in the preparation phase, reuse the creation of metric with good default values, can improve readability and learning for new contributors. Also created metrics with defaults that can parametrized in the act phase of the test can support the creation of good new tests by the contributors.

Which area does this relate to?

Metrics

Solution

• considerations :
  The Emf type could be introduced in a separate artifact Emf.ts
  The Emf type could be treated as given in pure Emf format without introducing inside this type new types created in powertools like the use case by introducing new types with the same meaning , MetricDefiniton.
  Is a property from emf needed in powertools , then it would improve handling the external dependency to the emf data model and improve readability when the property from the emf type is extracted directly from emf.

Does the MetricDefinition type belong to the types/Metrics.ts ?
This type could be defined inside the Metrics.ts implementation or in a location where it is not exported as part of the types of the metrics package .

type MetricDefinition = {
  Name: Emf['_aws']['CloudWatchMetrics'][0]['Metrics'][0]['Name']
  Unit: Emf['_aws']['CloudWatchMetrics'][0]['Metrics'][0]['Unit']
  StorageResolution: Emf['_aws']['CloudWatchMetrics'][0]['Metrics'][0]['StorageResolution']
} 

Additional
Is the mutable EmfOutput type used in Metrics.ts ? As is defined today it is mutable or does the Output mean readonly ?
The type of EmfOutput could be made explicit by refactoring as example to : ReadOnly<Emf> where Emf is the pure Data Model of EMF as defined by the EMF Schema and ReadOnly is a typerscript utility type (immutalble).

Does the ReadOnly<Emf> or EmfOutput type belong to the types/Metrics.ts ?
This type could be defined inside the Metrics.ts implementation or in a location where it is not exported as part of the types of the metrics package .

considerations:
The index Type for Stored Metrics can be refactored to a Record utility Type from Typescript with static structural support at compilation time.

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/types/Metrics.ts#L67-L69

And the mapReducer can be made more readable with a Record utility type for Stored Metrics, acting in a role of Store that can be used for mapping to an Emf result.

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/Metrics.ts#L370

And the return Type EmfOutput can be ReadOnly, is produced from serialize, improves readability and consistency.

consideration:
Tests for isNewMetric , comparing old and new mertics . Can a unit of an old metric be inconsistent ?

The isNew Metric is implemented as below . The if statement comparison (not equal) is used for old metrics.
However the units are implemented with structural typing support (enum and type combination).

https://github.com/awslabs/aws-lambda-powertools-typescript/blob/79a321b199ef51a024dc25b83673baf2eb03de69/packages/metrics/src/Metrics.ts#L477-L480

considerations:

• Use case: The method addMetric of Metrics which is defined in the interface MetricsInterface is extended with the new parameter resolution which is optional. The class Metrics that implements the MetricsInterface does not provide visual feedback about the new optional parameter resolution, hence optional.

However the IDE could have provided visual feedback if in the Metrics Tests, the metrics object is of type MetricsInterface. Hence the abstraction of MetricInterface is expected when extending the unit tests because of the introduction of new functionality that should be implemented in the concrete implementation of Metrics.

desribe(...) {
  test(...) {
    const metrics = new Metrics() as MetricsInterface ; 
    metrics.addMetric('test_name', MetricUnits.Seconds, MetricResolution.High);
  }
}

considerations:

• a guide or good example for the contributor to write comments in code is needed. The example may be a reference package , e.g Logger or Tracer which can improve the Metrics package for comments in code.
• reference example can indicate when to write examples , when to comment types , when to provide comment for optional parameters and default value.
• a balanced solution for rendering comments in code for using in IDE and type doc is needed.

considerations :
helper functions that can be called in one line for prepare of unit tests with meaningful name, like createMetric(), addHighResolutionMetric() that can be reused in tests and contributors can read the encapsulated code . That can improve readability and learning .

Acknowledgment

• This request meets Lambda Powertools Tenets
• Should this be considered in other Lambda Powertools languages? i.e. Python, Java, and .NET

Future readers

Please react with 👍 and your use case to help us understand customer demand.

[am29d]

Hey Niko, thanks for raising this issue 👍.

I assume this is a collection of observations you gained when working on metric resolution PR recently, right? I will dive a bit deeper into the area and will follow up with specifics to untangle all your points.

[niko-achilles]

Hey Alexander , yes it is from the contribution perspective , in the role of contributor experience .
Andreas gave me feedback in chat that 1, 3 and 4 would be better to explain more so that the issue reader can understand the why .

And for 6 there is an existing issue for testing in backlog : #163 .
It is about strategy and the point 6 of this issue can also be considered as part of strategy .

let me know how you would like handle the observations and any further information needed .

[niko-achilles]

Alexander i have updated the description of 1,2,3,4,5,6 regarding the observations from my metric resolution PR.
Additional to 6 , also Points 3,4 are related to tests.

So 3,4,6 are test observations that provided also as input information to the issue that has focus on testing Nr. 163.

and 1,2 relative to types and 5 relative to code comments.

[dreamorosi]

Hi Niko, thank you for your patience. I'm going to address the different points one by one:

1

I see your points and I think there's an opportunity to revisit all the types, their placement, their naming (i.e. MetricUnits vs MetricUnit), and generally speaking the boundaries as part of the v2 discussion that we have started. I have taken note of this item here.

Note
We don't have an ETA for the v2 just yet. Definitely we are looking at a few months from now at this point. This means that I'd prefer to address and refine items of that discussion at a later date, as right now we are focused on other topics and we won't be addressing discussions that might spur there. At the moment the idea is to simply gather ideas/proposals there.

Aside from the larger discussion that I have mentioned above, the linked PR makes EmfOutput already readonly as you suggested - thank you.

2

The serializeMetrics() method does a number of steps in its internal implementation, however I stand by the name that was given to it.

While it's true that the method does a number of things, none of these are side effect and all of them are necessary to achieve the final goal: serializing the metrics that are stored into an EMF-compliant blob.

For this reason I would prefer not exposing any internal implementation detail in the name nor in the docstrings. As part of the linked PR however I have added some comments to the method and tried to simplify its logic & types used a bit.

3

As you mentioned, the storeMetric() method, which is called by the addMetric() one, checks whether the metric being stored is new or not by calling a third method called isNewMetric.

When the metric is new - i.e. a metric with the same name is not present in the buffer - then we create a new entry complete with unit, value, and resolution. When instead the metric is not new - a.k.a. another metric with the same name was already added earlier - then we add the new value to the existing metric values array.

As part of the isNewMetric function, we also check if the unit is consistent with the unit of the stored metric with the same name. If the units are inconsistent, we throw an error as this is likely a bug or typo. This can happen if a metric is added without using the MetricUnit helper in JavaScript codebases.

For instance, let's take the JavaScript code below:

const { Metrics, MetricUnits } = require('@aws-lambda-powertools/metrics');

const metrics = new Metrics({ namespace: 'test' });

export const handler = async () => {
  metrics.addMetric('test', MetricUnits.Bits, 1); // Using `MetricUnits`
  metrics.addMetric('test', 'Bit', 1); // Writing the unit as a string "manually"

  metrics.publishStoredMetrics();
};

I don't have visibility over the amount of customers who use JavaScript and don't use an IDE that would flag this, however even if they might be a minority, the example above is why we need that check.

4

You are right, it appears that in the case of optional or extra properties, the interface doesn't help.

For instance, this correctly warns the user:

interface Something {
  myMethod(paramA: string): void
}

class Stuff implements Something {
  myMethod(paramA: string, paramB: number): void { // IDE complains as `paramB` is not part of the interface
    ...
  }
}

While this (which is similar to the case we are discussing), doesn't:

interface Something {
  myMethod(paramA: string, paramB: number): void
}

class Stuff implements Something {
  myMethod(paramA: string): void { // IDE complains 
    ...
  }
}

I have to admit that I am not 100% sure why this happens. I think it might be related to the tsconfig.json, but I am not 100% sure.

If there's a solution, I'd be open to discuss it, but the one you suggest (adding the as MetricsInterface to the tests) doesn't seem to fix the issue for me.

For now I'd like to defer the discussion to a later time, I have taken note in #617 to see if tsconfig is in fact the culprit.

5

You are right.

Comments in this utility are not uniform and could use improvement.

Contributors are expected to add comments and maintain existing ones up to date as they change the implementation. Priority is however given to public methods, while private methods are a strongly suggested nice-to-have.

The linked PR addresses this point entirely and adds docs for all methods of the class. Hope this helps.

6

I acknowledge and accept your feedback about the unit tests not being approachable enough for new contributors.

As you have already noticed, there's another ongoing work stream to improve the unit tests structure and style. This is a work that I have decided to prioritize as I considered that before making further improvements/changes, it was important to bring all unit tests to a common baseline.

After that PR is merged, we can potentially discuss adding more helper functions and even custom matchers - however I want to be clear since the beginning about a few aspects:

• generally speaking we prefer simplicity over terse, but over-engineered, solutions
• our time and resources are limited and at this point in time we would like to prioritize customer-facing features and discussions
• finally: a lot of this comes down to personal preference - I'm open to listen to the community as a whole, but we should all acknowledge that when it comes to styles we might not agree 100% of the time.

---

In the future, please consider opening issues that are more focused and clear in their intent. This will definitely help us maintainers and other community members to engage in a more timely manner.

In this case however team and I will assume part of the fault for failing to point this out early on and making you wait long. We'll try to do better in the future and either suggest to refine the issue or telling you early on that we are not planning on addressing it.

While many of the points brought up in the original issue were valid and valuable, they required a lot of heavy lifting from the reader to be understood, contextualized, and made actionable. It took me almost 5 hours of work to parse everything and come up with this response.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[dreamorosi]

Hello, just wanted to update the discussion to reflect the fact that the team has had a conversation with Niko and agreed that we will try to improve our processes around deciding what kind of topics/discussions are better suited for issues vs discussions.

We'll be updating our MAINTAINERS handbook in the coming days to codify guidance around using issues for more actionable and focused discussions, while also suggesting using GitHub Discussions for those conversations that require a more thoughtful and extended conversation.

[github-actions]

This is now released under v1.9.0 version!