fix: Improve telemetry allocations by murarustefaan · Pull Request #2185 · cloudquery/plugin-sdk

murarustefaan · 2025-06-06T12:55:02Z

This PR:

moves OTEL counter initialization to a single place
removes startTime and endTime non-counter metrics
introduces duration metric
removes client-level cardinality from the metrics, preventing memory accumulation

erezrokah

Maybe we could do this behind an option somehow? The code in https://github.com/cloudquery/cloudquery/blob/983062b2210f6a269743afda4c56c5df9e0cdc77/cli/internal/otel/receiver.go and https://github.com/DataDog/integrations-extras/blob/ecce46b7b5ec5cac1a509714b01da7fc394be083/cloudquery/assets/dashboards/cloudquery_syncs_insights.json relies on the metrics and probably other OTEL consumers.

murarustefaan · 2025-06-09T09:43:03Z

Maybe we could do this behind an option somehow?

I'm afraid not, sadly. Having client_id metric granularity means that for plugins like AWS, a single AWS account generates a metric cardinality of around 19-20k due to our multiplexing. This is totally against what opentelemetry recommends, a maximum metric cardinality of 2k.

I also do not see the reason why we need to expose the internal client_id implementations as separate metrics, since we already expose them as traces and spans (this PR only touches the metric implementation); we can just as easily update the DD dashboard & CLI to use the new metrics.

Flagging this behind an option would just postpone the issue, rather than fix it. 😞
I'm open to other suggestions, but realistically from pprof-ing my way trough this, reducing the metric cardinality is the only thing that will fix the memory usage (both in terms of in-memory metric storage as well as aggregation call allocations in the OTEL SDK).

See this related OTEL issue.

erezrokah · 2025-06-09T10:23:21Z

I also do not see the reason why we need to expose the internal client_id implementations as separate metrics

Yeah agree, I think the reason is that's how we saved pre-OTEL metrics (per client id) so OTEL code is aligned with that.

Can we try setting OTEL_GO_X_CARDINALITY_LIMIT per the suggestion in the issue? At least when running the platform

murarustefaan · 2025-06-09T10:35:21Z

scheduler/metrics/metrics.go

-	startTime, err := otel.Meter(OtelName).Int64Counter("sync.table.start_time",
-		metric.WithDescription("Start time of syncing a table"),
-		metric.WithUnit("ns"),
+	duration, err := otel.Meter(ResourceName).Int64Counter(durationMetricName,
+		metric.WithDescription("Duration of syncing a table"),
+		metric.WithUnit("ms"),
 	)
 	if err != nil {
 		return nil
 	}

-	endTime, err := otel.Meter(OtelName).Int64Counter("sync.table.end_time",
-		metric.WithDescription("End time of syncing a table"),
-		metric.WithUnit("ns"),
-	)
-
-	if err != nil {
-		return nil
-	}


This removes the 2 timestamp metrics, which did not really make sense as metrics since they were not increasing like a counter should (however, aggregations were still ran periodically for them, hence cardinality).

murarustefaan · 2025-06-09T10:39:21Z

Can we try setting OTEL_GO_X_CARDINALITY_LIMIT per the suggestion in the issue? At least when running the platform

Setting OTEL_GO_X_CARDINALITY_LIMIT is a separate thing, targeted to another repo.

Setting that will also have the effect that metrics will simply not be counted, therefore there will be missing data.

Implementing this changeset will solve the platform issues as well while not needed to set the custom OTEL flag since cardinality will be lower than that (number of tables).

erezrokah · 2025-06-09T10:53:52Z

Setting that will also have the effect that metrics will simply not be counted, therefore there will be missing data.

Yeah not saying we shouldn't fix the cause, just if the platform doesn't use the metrics, and we can set OTEL_GO_X_CARDINALITY_LIMIT to solve the memory issues that seems like an easy fix

przste-go

lgtm

przste-go · 2025-07-16T11:26:36Z

scheduler/metrics/metrics.go

+		m.measurements[table.Name].clients[client.ID()] = &measurement{duration: &durationMeasurement{}}
+	}
+	for _, relation := range table.Relations {
+		m.InitWithClients(invocationID, relation, clients)


Only question here. Didn't you want to get rid of per client otel metrics to reduce cardinality even further ?

That actually makes sense, initially I wanted to build this changeset in a way where we would have been able to detect per-invocation metrics; however, I missed that we have traces that are able to give us that data so I think it's safe to say we don't really need that level of granularity in metrics for now.

What about client.ID() ? Isn't table.Name enough if we have per client traces ?

The client_id is not exposed in the OTEL metrics, it's just used for tracking per-client resolver durations.

plugin-sdk/scheduler/metrics/metrics.go

Lines 88 to 90 in 9e30558

Set: attribute.NewSet(

attribute.Key("sync.table.name").String(tableName),

),

erezrokah

Looks good, did we run a sync with this? Wondering how the logs/traces/metrics look like now.

To confirm, the only user facing change is dropping StartTime/EndTime in favor of Duration?

murarustefaan · 2025-07-16T13:59:54Z

Looks good, did we run a sync with this? Wondering how the logs/traces/metrics look like now.

Yes, see the description of the CLI draft PR for before and after metric examples made in a backwards-com.
cloudquery/cloudquery#21048

To confirm, the only user facing change is dropping StartTime/EndTime in favor of Duration?

ClientID will also miss on plugin versions built on top of this. See the above PR.

…k in older versions

murarustefaan · 2025-07-17T08:55:21Z

OK, this is ready to merge.

I added back a static, empty client.id metric point so that previous CLI versions do not break when used with --table-metrics-location (they were trying to cast this to string without error checking and therefore it was panicking).

…c point granularity (#21048) Should go with cloudquery/plugin-sdk#2185 in order to allow the CLI to read metrics without the client_id metric point. [old CLI, old SDK](https://github.com/user-attachments/files/21236264/before.txt) [old CLI, new SDK](https://github.com/user-attachments/files/21289151/after.txt) [updated CLI, old SDK](https://github.com/user-attachments/files/21289612/after.txt) [updated CLI, new SDK](https://github.com/user-attachments/files/21236263/after.txt) The above showcase that Duration metrics are unavailable if using an old CLI version with newer plugin versions, but that's a downside we have to work with.

🤖 I have created a release *beep* *boop* --- ## [4.87.3](v4.87.2...v4.87.3) (2025-07-17) ### Bug Fixes * **deps:** Update module github.com/aws/aws-sdk-go-v2/service/licensemanager to v1.32.0 ([#2230](#2230)) ([28a1479](28a1479)) * Improve telemetry allocations ([#2185](#2185)) ([b07ce76](b07ce76)) * Upgrade golangci-lint to v2 ([#2228](#2228)) ([7fc238c](7fc238c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

github-actions bot added the fix label Jun 6, 2025

erezrokah reviewed Jun 9, 2025

View reviewed changes

murarustefaan commented Jun 9, 2025

View reviewed changes

github-actions bot added fix and removed fix labels Jun 9, 2025

murarustefaan marked this pull request as ready for review June 9, 2025 12:05

murarustefaan requested review from a team and jon-s58 June 9, 2025 12:05

murarustefaan force-pushed the fix/improve-telemetry branch from 129dd57 to 2123d20 Compare July 10, 2025 10:29

murarustefaan added 4 commits July 10, 2025 14:31

fix: Improve telemetry allocations

8ab8841

fix: Compute counters per-table and not per table per client

a1a5179

fix: working per-table & per-client duration

b8496f9

fix: tests, remove prometheus client

457f17a

murarustefaan force-pushed the fix/improve-telemetry branch from 2123d20 to 457f17a Compare July 10, 2025 11:31

murarustefaan added 2 commits July 10, 2025 14:42

fix: lower durations in tests

c1c41cd

fix: Ensure operations take longer than 1ms

0fb408d

murarustefaan requested review from erezrokah, maaarcelino and przste-go July 14, 2025 14:10

Merge branch 'main' into fix/improve-telemetry

79ebc60

murarustefaan mentioned this pull request Jul 15, 2025

feat: Support the new OTEL metric format removing the client_id metric point granularity cloudquery/cloudquery#21048

Merged

przste-go approved these changes Jul 16, 2025

View reviewed changes

przste-go reviewed Jul 16, 2025

View reviewed changes

murarustefaan added 2 commits July 16, 2025 16:02

fix: Remove invocationID from metric point attributes

fd813fc

fix: Remove invocationID from metrics constructor calls

9e30558

erezrokah approved these changes Jul 16, 2025

View reviewed changes

feat: Add back empty client.id metric point so that CLI does not brea…

4deada4

…k in older versions

murarustefaan added the automerge label Jul 17, 2025

Merge branch 'main' into fix/improve-telemetry

b6f3d15

kodiakhq bot merged commit b07ce76 into main Jul 17, 2025
10 of 11 checks passed

kodiakhq bot deleted the fix/improve-telemetry branch July 17, 2025 10:26

cq-bot mentioned this pull request Jul 17, 2025

chore(main): Release v4.87.3 #2229

Merged

	Set: attribute.NewSet(
	attribute.Key("sync.table.name").String(tableName),
	),

Conversation

murarustefaan commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

murarustefaan commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erezrokah commented Jun 9, 2025

Uh oh!

murarustefaan Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

murarustefaan commented Jun 9, 2025

Uh oh!

erezrokah commented Jun 9, 2025

Uh oh!

przste-go left a comment

Choose a reason for hiding this comment

Uh oh!

przste-go Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

murarustefaan Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

przste-go Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

murarustefaan Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

przste-go Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

erezrokah left a comment

Choose a reason for hiding this comment

Uh oh!

murarustefaan commented Jul 16, 2025

Uh oh!

murarustefaan commented Jul 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

murarustefaan commented Jun 6, 2025 •

edited

Loading

murarustefaan commented Jun 9, 2025 •

edited

Loading