feat: ucan stream consumer w3 accum store/add size #146

vasco-santos · 2023-02-22T09:05:29Z

Adds a consumer to UCAN stream kinesis where total size of store/add operations is tracked in the system. To achieve this tracking, we simply consume UCAN stream, filter store/add operations and update a DynamoDB table row incrementing previous value in batches.

This PR adds only one metric consumer. Follow up PRs for remaining systsem metrics will follow same pattern as what we decide here.

It includes:

DynamoDB Table w3metrics, with one row per each system metric
Default config for consumers - more details about those in feat: ucan stream consumer count uploads in space #118
Dead letter queue for failures
Integration tests to check metrics are updated running the UCAN Stream consumer

Note that:

Numbers can be positive, negative, or zero. Numbers can have up to 38 digits of precision.

See DynamoDB Tables content after integration tests ran:

Closes #145

seed-deploy · 2023-02-22T09:16:19Z

View stack outputs

pr146-w3infra-CarparkStack

Name Value

BucketName carpark-pr146-0

Region us-east-2
pr146-w3infra-SatnavStack

Name Value

BucketName satnav-pr146-0

Region us-east-2
pr146-w3infra-UploadApiStack

Name Value

ApiEndpoint https://pyi3yh2vid.execute-api.us-east-2.amazonaws.com

CustomDomain https://pr146.up.web3.storage
pr146-w3infra-BusStack
pr146-w3infra-ReplicatorStack
pr146-w3infra-UcanInvocationStack
pr146-w3infra-UploadDbStack

alanshaw · 2023-02-22T14:29:51Z

stacks/upload-db-stack.js

+  /**
+   * This table tracks w3 wider metrics.
+   */
+  const w3MetricsTable = new Table(stack, 'w3-metrics', w3MetricsTableProps)


Why does the table have w3- prefix? The others do not...

Should this be admin-metrics instead to signify it's not user facing?

alanshaw · 2023-02-22T14:35:04Z

test/upload-api.test.js

+
+  // Get metrics before upload
+  const beforeOperationW3Metrics = await getW3Metrics(t)
+  const beforeW3AccumulatedSize = beforeOperationW3Metrics.find(row => row.name === W3_METRICS_NAMES.STORE_ADD_ACCUM_SIZE)


Please can we have a separate test('POST / updates metrics') for example?

I wanted to do what you suggested and should have documented decision in PR description!

This is the integration tests and therefore we write real CAR files (a new one per PR/commit) that will live in S3/R2. Adding more individual tests will lead into more and more files that we will need to pay for storage + egress just for testing.

Probably I can suggest renaming this file to integration.test.js and make test names more appropriate to include everything. Thoughts?

Ohhh ok, yeah renaming will be fine!

alanshaw · 2023-02-22T14:41:38Z

ucan-invocation/functions/metrics-accumulated-size.js

+ */
+export async function updateAccumulatedSize (ucanInvocations, ctx) {
+  const invocationsWithStoreAdd = ucanInvocations.filter(
+    inv => inv.value.att.find(a => a.can === STORE_ADD)


Should we consider also store/remove into this calculation? Or, perhaps better - collect a separate metric that for store/remove-accumulated-size (followup PR)?

yes, follow up PR with your suggestion. The goal of this PR is to get a "this is the way we should do things". Then follow up PRs with consumers for each metrics will be just follow same pattern

Added in tracking #117

alanshaw · 2023-02-22T14:43:13Z

test/upload-api.test.js

+    const afterOperationW3Metrics = await getW3Metrics(t)
+    const afterW3AccumulatedSize = afterOperationW3Metrics.find(row => row.name === W3_METRICS_NAMES.STORE_ADD_ACCUM_SIZE)
+
+    return afterW3AccumulatedSize?.value > beforeW3AccumulatedSize.value


Please can we test the real value after storing multiple times?

Goal here was to create a balance to get to a point where we do not have flaky tests. When this integration tests run in the PR scope is actually fine. However, they also run in staging. Making such assumption in staging can be problematic because of parallel engineer testing something in staging or when we hook up other tests in staging that write stuff there (for example, for w3infra)...

We can make a conditional more explicit. If is staging, just accept > and if it is PR expect real value different.

What do you think?

Did this in commit

alanshaw · 2023-02-22T14:48:31Z

ucan-invocation/constants.js

+
+// Metrics
+export const W3_METRICS_NAMES = {
+  STORE_ADD_ACCUM_SIZE: `${STORE_ADD}-accumulated-size`


Meaning is basically the same but I would have gone for total over accumulated for succinctness - just wondering if there was a different reason?

changed to total everywhere

ucan-invocation/tables/index.js

alanshaw · 2023-02-22T14:52:59Z

ucan-invocation/tables/w3-metrics.js

+     *
+     * @param {Capabilities} operationsInv
+     */
+    incrementAccumulatedSize: async (operationsInv) => {


This should either be specific to store add i.e. incrementStoreAddAccumulatedSize or take the key as a param.

alanshaw · 2023-03-06T15:45:05Z

stacks/upload-db-stack.js

+  /**
+   * This table tracks w3 wider metrics.
+   */
+  const adminMetricsTable = new Table(stack, 'metrics', adminMetricsTableProps)


Suggested change

const adminMetricsTable = new Table(stack, 'metrics', adminMetricsTableProps)

const adminMetricsTable = new Table(stack, 'admin-metrics', adminMetricsTableProps)

alanshaw · 2023-03-06T15:45:46Z

test/integration.test.js

+
+  // Get metrics before upload
+  const beforeOperationMetrics = await getMetrics(t)
+  const beforeAccumulatedSize = beforeOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)


Suggested change

const beforeAccumulatedSize = beforeOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)

const beforeAccumulatedSize = beforeOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_TOTAL_SIZE)

alanshaw · 2023-03-06T15:46:55Z

test/integration.test.js

+
+    // If staging accept more broad condition given multiple parallel tests can happen there
+    if (stage === 'staging') {
+      return afterAccumulatedSize?.value > beforeAccumulatedSize.value


Suggested change

return afterAccumulatedSize?.value > beforeAccumulatedSize.value

return afterAccumulatedSize?.value >= beforeAccumulatedSize.value + carSize

alanshaw · 2023-03-06T15:47:22Z

test/integration.test.js

+  // Check metrics were updated
+  beforeAccumulatedSize && await pWaitFor(async () => {
+    const afterOperationMetrics = await getMetrics(t)
+    const afterAccumulatedSize = afterOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)


Suggested change

const afterAccumulatedSize = afterOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)

const afterTotalSize = afterOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)

vasco-santos · 2023-03-06T16:00:39Z

@alanshaw addresses last comments, also renaming lambda consumer to include store-add to better separate from future ones. Also, I named things putting total in the end to be coherent with Prometheus best practises so that once we expose these metrics we can keep everything consistent

feat: ucan stream consumer accum store size

8b6b519

seed-deploy bot temporarily deployed to pr146 February 22, 2023 09:05 Inactive

vasco-santos marked this pull request as ready for review February 22, 2023 09:23

vasco-santos changed the title ~~feat: ucan stream consume w3r accum store size~~ feat: ucan stream consumer w3 accum store size Feb 22, 2023

vasco-santos changed the title ~~feat: ucan stream consumer w3 accum store size~~ feat: ucan stream consumer w3 accum store/add size Feb 22, 2023

alanshaw requested changes Feb 22, 2023

View reviewed changes

heyjay44 mentioned this pull request Feb 22, 2023

UCAN Log stream consumers #117

Closed

18 tasks

vasco-santos requested review from alanshaw and removed request for alanshaw February 23, 2023 13:06

seed-deploy bot temporarily deployed to pr146 February 23, 2023 17:21 Inactive

vasco-santos force-pushed the feat/ucan-stream-consumer-accum-store-size branch from ed164c6 to 02d4ee1 Compare February 23, 2023 17:25

seed-deploy bot temporarily deployed to pr146 February 23, 2023 17:30 Inactive

chore: address review comments

bc58362

vasco-santos force-pushed the feat/ucan-stream-consumer-accum-store-size branch from 02d4ee1 to bc58362 Compare February 23, 2023 18:04

seed-deploy bot temporarily deployed to pr146 February 23, 2023 18:04 Inactive

vasco-santos requested a review from alanshaw February 23, 2023 18:38

alanshaw requested changes Mar 6, 2023

View reviewed changes

chore: address review comments

40d2189

seed-deploy bot temporarily deployed to pr146 March 6, 2023 15:57 Inactive

vasco-santos requested a review from alanshaw March 6, 2023 16:09

alanshaw approved these changes Mar 6, 2023

View reviewed changes

vasco-santos merged commit 9c64395 into main Mar 6, 2023

vasco-santos deleted the feat/ucan-stream-consumer-accum-store-size branch March 6, 2023 16:25

vasco-santos mentioned this pull request Mar 9, 2023

[Tracking] w3up phase 3 storacha/w3up#221

Closed

83 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ucan stream consumer w3 accum store/add size #146

feat: ucan stream consumer w3 accum store/add size #146

vasco-santos commented Feb 22, 2023 •

edited

Loading

seed-deploy bot commented Feb 22, 2023

alanshaw Feb 22, 2023

alanshaw Feb 23, 2023

alanshaw Feb 22, 2023

vasco-santos Feb 22, 2023

alanshaw Feb 22, 2023

alanshaw Feb 22, 2023

vasco-santos Feb 22, 2023

vasco-santos Feb 23, 2023

alanshaw Feb 22, 2023

vasco-santos Feb 22, 2023

vasco-santos Feb 23, 2023

alanshaw Feb 22, 2023

vasco-santos Feb 23, 2023

alanshaw Feb 22, 2023

alanshaw Mar 6, 2023

alanshaw Mar 6, 2023

alanshaw Mar 6, 2023

alanshaw Mar 6, 2023

vasco-santos commented Mar 6, 2023

	const adminMetricsTable = new Table(stack, 'metrics', adminMetricsTableProps)
	const adminMetricsTable = new Table(stack, 'admin-metrics', adminMetricsTableProps)

	const beforeAccumulatedSize = beforeOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)
	const beforeAccumulatedSize = beforeOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_TOTAL_SIZE)

	return afterAccumulatedSize?.value > beforeAccumulatedSize.value
	return afterAccumulatedSize?.value >= beforeAccumulatedSize.value + carSize

	const afterAccumulatedSize = afterOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)
	const afterTotalSize = afterOperationMetrics.find(row => row.name === METRICS_NAMES.STORE_ADD_ACCUM_SIZE)

feat: ucan stream consumer w3 accum store/add size #146

feat: ucan stream consumer w3 accum store/add size #146

Conversation

vasco-santos commented Feb 22, 2023 • edited Loading

seed-deploy bot commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Mar 6, 2023

vasco-santos commented Feb 22, 2023 •

edited

Loading