feat: basic egress tracking prometheus metrics by volmedo · Pull Request #6 · storacha/etracker

volmedo · 2025-10-06T08:24:50Z

No description provided.

alanshaw

I've left comments on parts of the code I don't think were meant to be in this PR...

alanshaw · 2025-10-06T11:37:05Z

Suggestion: trigger this when you get a request, unless it is already running.

you mean not starting the consolidator (https://github.com/storacha/etracker/pull/6/files#diff-24d24419fa40f605bd077ae0a3b7e2bb7f703deb66475aa1a040798bee9bfb1aR138) until we get the first request?

No I mean instead of running on an interval - just just a request to trigger the run.

so consolidate on every track request? I think processing them in batches might be slightly more efficient?

alanshaw · 2025-10-06T11:42:52Z

Something more like... "ignoring unsuccessful retrieval: %s", receipt.Ran()

the error happens when trying to rebind the receipt, so I think it means the receipt is not of the expected type, no?

It being a retrieval receipt, but an unsuccessful one, is a different case that I'll check in validateReceipt.

alanshaw · 2025-10-06T11:43:22Z

This should be issuing a receipt with the errors no?

true, this is not an error processing a single receipt but an error that prevents the consolidation of the full batch

alanshaw · 2025-10-06T11:50:38Z

Personally I'd register a handler and call it - then you don't need to do things like issue a receipt, you can just return the ok/error type from the handler and store the returned receipt.

Note: it doesn't need to go over the network.

Right now everything looks a little weird because you're kinda manually executing the invocation.

alanshaw · 2025-10-06T11:52:09Z

It's a lot of data to buffer in memory - basically the size of a batch. Why not return a iter.Seq?

🤦🏻 I always forget about iterators. Good catch, thanks.

alanshaw · 2025-10-06T11:54:00Z

 	}

 	// TODO: do more validation here.
 	// At the very least that the invocation is a retrieval invocation and the audience is the node


You should invoke the validator, which will check the delegation chain.

wait, how do I do that?

https://github.com/storacha/go-ucanto/blob/a90e4511ad2c7513e6f5d631ee7a51c349a274c3/server/handler.go#L60

captured for posterity here #12

alanshaw · 2025-10-06T11:54:54Z

List?

Suggested change

List(ctx context.Context, nodeDID did.DID) ([]ConsolidatedRecord, error)

alanshaw · 2025-10-06T11:57:06Z

Minor, and partly because I think Decode works with strings also, but we usually parse strings and decode bytes.

yeah, Decode accepts a string (it wouldn't compile otherwise) and Parse essentially calls Decode when the param is a string, but I'll update

alanshaw · 2025-10-06T11:59:50Z

+		return err
+	}
+
+	metrics.TrackedBatchesPerNode.WithLabelValues(nodeDID.String()).Inc()


I think you need to increment in the DB also and when you start the service, read it from the DB and set the counter value. Otherwise when you restart the service your counts will be off.

I don't think that's necessary. Prometheus handles resets of the counter in functions like rate or increase. In a sense, the actual absolute value of the counter doesn't really matter.

volmedo · 2025-10-06T12:40:17Z

I've left comments on parts of the code I don't think were meant to be in this PR...

sorry, I forgot to rebase the branch onto main after merging the branch this PR was targeting. The good thing about this is that I got your comments on #5 🙂. I'll take care of them in a follow up.

frrist · 2025-10-06T15:59:14Z

@@ -0,0 +1,26 @@
+package metrics


Any reason this can't use the opentelemetry go sdk?

no, I just wanted something quick and this is how I set up prometheus metrics endpoints in the past. As opposed to piri's case, this is a service we own so I can add secrets, and the endpoint is going to be scrapped by Grafana, so I just went the default route.

I've been thinking (hoping even) we could move toward standardizing on OpenTelemetry across services - indexing-service, Piri, and (soon) guppy all use it.
The Go SDK has a native Prometheus exporter that works nearly the same way, so I don't see it adding much more complexity.

That said, totally get wanting to move fast here. Happy to help with OTel if you're interested, but no worries either way

take a look at 531320f and let me know if it matches what you had in mind

ConsolidatedBytesPerNode could probably be an integer counter, will leave it up to you. Looks good otherwise - thanks!

ah, good catch, thank you!

frrist · 2025-10-06T20:42:29Z

+		// Increment consolidated bytes counter for this node
+		metrics.ConsolidatedBytesPerNode.WithLabelValues(record.NodeID.String()).Add(float64(totalBytes))


nit: probably cleaner to handle the recording keeping of this metric within the DB method. i.e. within consolidatedTable.Add

I disagree with this, because the metric is not related with how the data is stored IMO. If we had more than one implementation of ConsolidatedTable we wouldn't want to have all and each of them to publish the metric.

frrist · 2025-10-06T20:45:17Z

+		return err
+	}
+
+	metrics.TrackedBatchesPerNode.WithLabelValues(nodeDID.String()).Inc()


Similar to my comment elsewhere, could do this here: https://github.com/storacha/etracker/blob/main/internal/db/egress/dynamodb.go#L70

similar comment, similar reply 😛

volmedo · 2025-10-07T15:44:37Z

now that feedback not related with the metrics endpoint is been addressed in #10 I'm rebasing this on main to focus on the metrics changes. Sorry for the hassle.

frrist · 2025-10-07T15:56:42Z

+	startCmd.Flags().String(
+		"grafana-metrics-token",
+		"",
+		"Grafana metrics token",
+	)
+	cobra.CheckErr(viper.BindPFlag("grafana_metrics_token", startCmd.Flags().Lookup("grafana-metrics-token")))
+


nit: Would prefer we use (add) an env var here. Or maybe we already have an env var for this, in which case disregard my comment; CLI args are visible in ps output and shell history, Env vars aren't perfect but they're significantly less exposed.

it's an env var, storoku automatically adds env vars to make secrets available to the service. I created a flag with the same name so that the env var is picked up automatically. Is there an option to make viper read the env var without exposing a CLI arg?

Yeah, https://pkg.go.dev/github.com/spf13/viper#BindEnv like you are using below. This isn't a blocking comment, just a suggestion for future changes.

oh, I thought the existence of a flag was a pre-requisite for BindEnv to work. So I can remove the startCmd.Flags().String("metrics-auth-token", ...) definition and just issue a viper.BindEnv("metrics_auth_token")? Nice.

roughly yes, it depends on how viper is configured in this project.

Based on https://github.com/storacha/etracker/blob/main/cmd/etracker/main.go#L45 that would bind the metrics_auth_token viper key to ETRACKER_METRICS_AUTH_TOKEN

frrist · 2025-10-07T15:59:04Z

+		// Increment consolidated bytes counter for this node
+		metrics.ConsolidatedBytesPerNode.Add(ctx, int64(totalBytes), metric.WithAttributes(attribute.String("node_id", record.NodeID.String())))
+


Should this happen after we issue the receipt? Ensuring we only count a metric if all operations in the loop are successful?

yes, and it will. After the changes in #10 issuing the receipt manually is not necessary anymore.

alanshaw · 2025-10-07T16:15:22Z

No I mean instead of running on an interval - just just a request to trigger the run.

alanshaw · 2025-10-07T16:20:35Z

 TF_VAR_private_key= # private_key or your env -- do not commit to repo!
 TF_VAR_allowed_account_id=505595374361
 TF_VAR_region=us-west-2
+TF_VAR_grafana_metrics_token= # enter a value for GRAFANA_METRICS_TOKEN secret


🤷‍♂️ the endpoint returns metrics in prometheus format - the fact that we use grafana to scrape/graph it is implementation detail. I would call this something more like METRICS_AUTH_TOKEN.

alanshaw · 2025-10-07T16:22:31Z

 	}

 	// TODO: do more validation here.
 	// At the very least that the invocation is a retrieval invocation and the audience is the node


https://github.com/storacha/go-ucanto/blob/a90e4511ad2c7513e6f5d631ee7a51c349a274c3/server/handler.go#L60

volmedo self-assigned this Oct 6, 2025

volmedo changed the base branch from main to vic/feat/consolidator-mvp October 6, 2025 08:25

Base automatically changed from vic/feat/consolidator-mvp to main October 6, 2025 09:41

alanshaw requested changes Oct 6, 2025

View reviewed changes

frrist reviewed Oct 6, 2025

View reviewed changes

volmedo mentioned this pull request Oct 7, 2025

feat: consolidator MVP follow ups #10

Merged

volmedo added 4 commits October 7, 2025 17:45

feat: add basic prometheus metrics

a06fffd

metrics endpoint does bearer token auth

b07cc3c

use OTel SDK's prom exporter

c331e15

ConsolidatedBytesPerNode is an integer

7ce4282

volmedo force-pushed the vic/feat/basic-metrics branch from cad0d3c to 7ce4282 Compare October 7, 2025 15:46

go mod tidy

d823d74

frrist approved these changes Oct 7, 2025

View reviewed changes

alanshaw approved these changes Oct 7, 2025

View reviewed changes

remove references to Grafana

c4ae63d

volmedo mentioned this pull request Oct 7, 2025

Implement proper retrieval receipt validation #12

Closed

volmedo added 3 commits October 8, 2025 09:46

Merge branch 'main' into vic/feat/basic-metrics

270a25e

use WithAttributeSet instead of WithAttribute

59993ff

metrics auth token is only an env var

2121baa

volmedo merged commit 827cb4d into main Oct 8, 2025
4 of 5 checks passed

volmedo deleted the vic/feat/basic-metrics branch October 8, 2025 11:17


	List(ctx context.Context, nodeDID did.DID) ([]ConsolidatedRecord, error)

		// Increment consolidated bytes counter for this node
		metrics.ConsolidatedBytesPerNode.WithLabelValues(record.NodeID.String()).Add(float64(totalBytes))

		// Increment consolidated bytes counter for this node
		metrics.ConsolidatedBytesPerNode.Add(ctx, int64(totalBytes), metric.WithAttributes(attribute.String("node_id", record.NodeID.String())))

Conversation

volmedo commented Oct 6, 2025

Uh oh!

alanshaw left a comment

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!

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!

volmedo commented Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volmedo commented Oct 7, 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!