chore: introduce metrics change #307

josecorella · 2025-09-15T16:05:33Z

For best reading and commenting experience, I suggest splitting your window in two; the review page and the rendered page.
Here are the rendered files:

Goals for 9-15-2025 Spec Review:

Agreement on optional metrics agents and how they will impact existing APIs
Agreement that Metrics Agent Interface and Implementation will be implemented in Dafny but only as wrappers and provide extern implementations to make moving off of Dafny easier.
Agreement on interface supported operations.

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Check any applicable:

Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

kessplas · 2025-09-15T20:07:40Z

changes/2025-09-12_introduce-metrics-interface/background.md

+metrics. Customers can then ask for updates to the implementations
+CT provides or customers can go an implement their own interfaces that are fine-tuned
+to their use cases.
+


Is there a middle ground? For example, logging information locally without uploading it anywhere.

kessplas · 2025-09-15T20:09:02Z

changes/2025-09-12_introduce-metrics-interface/background.md

+will feel like; getting no choice on the matter and opting to not upgrade.
+Going from never emitting metrics to always emitting them says to customers
+that their application no matter its use case will always benefit from metrics.
+Without letting customers make that choice, CT looses hard earned customer trust.


Suggested change

Without letting customers make that choice, CT looses hard earned customer trust.

Without letting customers make that choice, CT loses hard earned customer trust.

kessplas · 2025-09-15T20:09:31Z

changes/2025-09-12_introduce-metrics-interface/background.md

+### Issue 1: What will be the default behavior?
+
+As a client-side encryption library CT should be as cautious as possible.
+Customers of CT libraries should be on the driver seat and determine for


Suggested change

Customers of CT libraries should be on the driver seat and determine for

Customers of CT libraries should be in the driver's seat and determine for

english is silly

kessplas · 2025-09-15T20:09:58Z

changes/2025-09-12_introduce-metrics-interface/background.md

+become unsupported.
+
+Additionally, requiring customers to start emitting metrics
+almost certainly guarantees a breaking change across supported libraries.


I don't think this is true

How so? If we require that all customer facing APIs now require a metric agent, then you can't pick up this change in a minor version because you are required to make a code change.

Well, not that I am advocating for this, but we could just provide a metric agent.
I am against it, but there is probably a way to do this in a non-breaking way.

oh sure, I apparently forgot this was in the context of specific API changes, I retract my objection

seebees · 2025-09-15T20:09:59Z

changes/2025-09-12_introduce-metrics-interface/change.md

+at customer facing API call sites.
+
+Currently, the ESDK client APIs models are defined [here](https://github.com/aws/aws-encryption-sdk/blob/mainline/AwsEncryptionSDK/dafny/AwsEncryptionSdk/Model/esdk.smithy#L60-L126).
+This change would see that the client APIs be changed as follows:


This can't be the only change. I like the detail, but we should explicitly add the CMM/Keyring inputs to this list.

seebees · 2025-09-15T20:10:59Z

changes/2025-09-12_introduce-metrics-interface/change.md

+non-breaking way as the Metrics Agent will be an optional parameter
+at customer facing API call sites.
+
+Currently, the ESDK client APIs models are defined [here](https://github.com/aws/aws-encryption-sdk/blob/mainline/AwsEncryptionSDK/dafny/AwsEncryptionSdk/Model/esdk.smithy#L60-L126).


I assume that the DB-ESDK item encryptor would be similar.
What about the DDB client interface?
Also, are there plans for S3EC?

seebees · 2025-09-15T20:11:42Z

framework/metrics-agent.md

+### label
+
+A label is a string that is used
+as a an attribute name to aggregate


Suggested change

as a an attribute name to aggregate

as an attribute name to aggregate

lucasmcdonald3 · 2025-09-15T20:02:57Z

changes/2025-09-12_introduce-metrics-interface/background.md

+to either a local application or to an observability service like AWS CloudWatch.
+
+As client side encryption libraries emitting metrics must be done carefully as
+to avoid accidentally [leaking](https://github.com/aws/aws-encryption-sdk-python/pull/105/files) any information related to the plaintext that could lead to a


I'm not sure if we want to link to this :)

I am planning on removing it before merging but wanted it here for folks to get context.

lucasmcdonald3 · 2025-09-15T20:05:50Z

changes/2025-09-12_introduce-metrics-interface/change.md

+## Summary
+
+Existing users of Crypto Tools (CT) libraries do no have any insights as to
+how the librar(y/ies) behave(s) in their application.


Suggested change

how the librar(y/ies) behave(s) in their application.

how the libraries behave in their application.

simplify, we know what you mean

lucasmcdonald3 · 2025-09-15T20:12:20Z

framework/metrics-agent.md

+  @required
+  label: String,
+  @required
+  duration: Long,  // Duration in milliseconds


I like Ryan's Duration model in Lockbox that would allow users to specify the unit: https://github.com/aws/private-aws-encryption-sdk-dafny-staging/blob/seebees/lockbox-smithy/AwsLockbox/dafny/AwsLockbox/Model/structures.smithy#L17-L22

texastony · 2025-09-15T20:13:07Z

@josecorella I have a meta/macro question.
Is there a proposal doc that accompanies these change docs?

I appreciate that the Background doc highlights issues and alternatives, but I feel like we a missing a "User Stories" document, that can be used to measure success criteria and what are the table stakes of this work.

It is also possible I just missed such a proposal doc; but without it, it is difficult to work backwards.

kessplas · 2025-09-15T20:13:15Z

changes/2025-09-12_introduce-metrics-interface/background.md

+Additionally, requiring customers to start emitting metrics
+almost certainly guarantees a breaking change across supported libraries.
+
+### Issue 2: Should Data Plane APIs fail if metrics fail to publish?


What about latency?

This is somewhat relevant to the discussion around availability, particularly in technical terms.

What I'm getting at is, are we planning to make blocking calls to CT or wherever, or have a separate thread/pool that does so periodically? Do we get this for free from the CT metrics agent (or whatever) or do we have to write this ourselves?

In any case we should specify what the bar is here for performance.

+1 . We should delegate the whole metric stuff to some known frame work. Maintaining this would be difficult and in any case we shouldn't be reinventing the wheel.

seebees · 2025-09-15T20:14:08Z

framework/metrics-agent.md

+operation AddDate {
+  input: AddDateInput,
+  output: AddOutput,
+  errors: [MetricsPutError]


Nit: You say that the interface should not error, but you have errors here.

seebees · 2025-09-15T20:14:43Z

framework/metrics-agent.md

+// Common output structure
+structure AddOutput {}


I get why you might optimize this. But is this really the best choice? why not have a output per operation?

texastony · 2025-09-15T20:17:03Z

changes/2025-09-12_introduce-metrics-interface/background.md

+## Issues and Alternatives
+
+Crypto Tools (CT) publishes software libraries. The latest
+versions of these libraries have no logging or metrics publishing


Nit/Issue: We have not defined metrics.
While the definition might seem obvious to some, metrics can either be a description of the library's performance in the customer's application OR they could be telementry on the usage of Crypto Tools products.

Having read the docs, it is clear that is NOT telementry, and maybe it is only Tony Knapp who gets the two confused, but we could clarify this.

kessplas · 2025-09-15T20:18:31Z

changes/2025-09-12_introduce-metrics-interface/change.md

+
+This change will allow Crypto Tools to introduce a Metrics Agent in a
+non-breaking way as the Metrics Agent will be an optional parameter
+at customer facing API call sites.


Should we support providing a metrics agent to the client constructor? Such that the same instance would then be used for each API call.

I don't necessarily think we should but it might be worth discussing here.

ShubhamChaturvedi7 · 2025-09-15T20:20:17Z

changes/2025-09-12_introduce-metrics-interface/change.md

+      .MaterialProvidersConfig(MaterialProvidersConfig.builder().build())
+      .build();
+
+final IKeyring rawAesKeyring = matProv.CreateRawAesKeyring(keyringInput);


Why are we implementing our own metric agent? I'm pretty sure there exists some framework which we can leverage. Customers can optionally provide the agent, or we do no-op (like logging frameworks such as slf4j etc).

kessplas · 2025-09-15T20:35:25Z

changes/2025-09-12_introduce-metrics-interface/change.md

+  algorithmSuiteId: aws.cryptography.materialProviders#ESDKAlgorithmSuiteId,
+
+  frameLength: FrameLength,
+


I guess this probably isn't possible in Smithy/Smithy-Dafny but we might want to consider having a more open-ended "request override" option for things like this. (This is the same way the SDK does it.)

texastony · 2025-09-16T00:36:50Z

Potential Issue/Alternative/User Story:

Users of the MPL/ESDK/DB-ESDK are also users of the AWS SDKs. The AWS SDKs have established logging and metric interfaces.
AWS Crypto Tools products are AWS Products, just like the AWS SDKs.

There likely is an implicit customer expectation that Crypto Tools products behave and appear to be consistent with the AWS SDKs.

Therefore, I suggest we carefully evaluate if we can utilize the SDKs metric and logging tooling, and offer a customer experience that closely mimics the SDKs experience.

The current collection of docs does not state this as a goal, but it does leave it open as an opportunity.

i.e: the proposed metric interface could wrap an SDK metric class.

josecorella added 7 commits September 5, 2025 15:53

chore: introduce metrics interface in change doc

ccd27f7

finished background doc

402d3cd

more work on the change doc

0fcc10e

add metrics-agent interface file

97380ef

finish first draft of metrics interface

a42a891

prettify

61fe175

update

e95b0f2

josecorella requested a review from a team as a code owner September 15, 2025 16:05

kessplas reviewed Sep 15, 2025

View reviewed changes

seebees reviewed Sep 15, 2025

View reviewed changes

lucasmcdonald3 reviewed Sep 15, 2025

View reviewed changes

kessplas reviewed Sep 15, 2025

View reviewed changes

seebees reviewed Sep 15, 2025

View reviewed changes

texastony reviewed Sep 15, 2025

View reviewed changes

kessplas reviewed Sep 15, 2025

View reviewed changes

ShubhamChaturvedi7 reviewed Sep 15, 2025

View reviewed changes

kessplas reviewed Sep 15, 2025

View reviewed changes

	Without letting customers make that choice, CT looses hard earned customer trust.
	Without letting customers make that choice, CT loses hard earned customer trust.

	Customers of CT libraries should be on the driver seat and determine for
	Customers of CT libraries should be in the driver's seat and determine for

	as a an attribute name to aggregate
	as an attribute name to aggregate

	how the librar(y/ies) behave(s) in their application.
	how the libraries behave in their application.

		algorithmSuiteId: aws.cryptography.materialProviders#ESDKAlgorithmSuiteId,

		frameLength: FrameLength,

chore: introduce metrics change #307

Are you sure you want to change the base?

chore: introduce metrics change #307

Uh oh!

Conversation

josecorella commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Check any applicable:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

texastony commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

texastony commented Sep 16, 2025

Uh oh!

Uh oh!

josecorella commented Sep 15, 2025 •

edited

Loading