Skip to content

feat(v2): implement basic metric recording function#487

Merged
quartzmo merged 5 commits intogoogleapis:mainfrom
westarle:westarle/metrics-record-basic-v4
Mar 24, 2026
Merged

feat(v2): implement basic metric recording function#487
quartzmo merged 5 commits intogoogleapis:mainfrom
westarle:westarle/metrics-record-basic-v4

Conversation

@westarle
Copy link
Copy Markdown
Contributor

@westarle westarle commented Mar 17, 2026

This PR introduces the internal recordMetric function, which emits the gcp.client.request.duration metric.

It also introduces two exported APIs:

  1. apierror.Message which returns the raw string (e.g. the "server error" part of an HTTP response that doesn't have an error.)
  2. gax.ParseTelemetryErrorInfo takes a context and error and populates a struct with all the required error information for tracing, logging and metrics.

Subsequent PRs will hook this function into gax.Invokeand expand the recorded attributes with dynamic error statuses and transport metadata.

We would add this to invoke:

	if IsFeatureEnabled("METRICS") {
		start := time.Now()
		defer func() {
			recordMetric(ctx, settings, time.Since(start), err)
		}()
	}

64a4698

@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from 91da9bc to c96eac9 Compare March 17, 2026 21:19
@westarle westarle marked this pull request as ready for review March 18, 2026 03:17
@westarle westarle requested a review from a team as a code owner March 18, 2026 03:17
Comment thread v2/telemetry_test.go Outdated
point := histo.DataPoints[0]

// Verify the duration value corresponds to the sum.
if point.Sum != 1.5 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to define an epsilon error bound since we're testing floating point equality? We're probably okay since we're only testing a literal duration transformation, but if we find jitter is exposed from the otel layer it might be something to add later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea, I added it.

@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from d8b1638 to f05a22a Compare March 19, 2026 04:42
@westarle westarle requested a review from shollyman March 19, 2026 04:42
@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch 10 times, most recently from 85eb494 to 53a25a1 Compare March 22, 2026 17:55
Copy link
Copy Markdown
Member

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR copies/adapts logic from cloud.google.com/go/auth (googleapis/google-cloud-go#14133). Gax seems to me to be the better home for this logic. Can we export it in this PR and then update cloud.google.com/go/auth to use the Gax exports?

Comment thread v2/telemetry.go Outdated
Comment thread v2/telemetry.go Outdated
Comment thread v2/telemetry.go Outdated
@quartzmo
Copy link
Copy Markdown
Member

Subsequent PRs will hook this function into gax.Invokeand expand the recorded attributes with dynamic error statuses and transport metadata.

Can you provide a code snippet preview in the PR description of how you think Invoke/invoke will call recordMetric? That will help me with the review.

@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from 53a25a1 to d1bc0d6 Compare March 23, 2026 20:42
@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from d1bc0d6 to b055edf Compare March 23, 2026 21:09
Comment thread v2/telemetry.go Outdated
Comment thread v2/telemetry.go Outdated
@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch 5 times, most recently from 540e394 to e790ab5 Compare March 24, 2026 02:38
@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from e790ab5 to 80c44d0 Compare March 24, 2026 02:53
@westarle westarle force-pushed the westarle/metrics-record-basic-v4 branch from 5356c09 to 55a8f4d Compare March 24, 2026 16:28
Comment thread v2/apierror/apierror.go Outdated
Comment thread v2/telemetry.go
Metadata map[string]string

// _ struct{} prevents unkeyed struct literals, ensuring backwards
// compatibility when new fields are added in the future.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is good practice.

Comment thread v2/telemetry.go
Comment thread v2/telemetry.go
//
// Experimental: This function is experimental and may be modified or removed in future versions,
// regardless of any other documented package stability guarantees.
func ExtractTelemetryErrorInfo(ctx context.Context, err error) TelemetryErrorInfo {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a pure parser that now returns a clean data struct from inputs, this is a big improvement over the original version in auth. Nice work!

@westarle westarle requested a review from quartzmo March 24, 2026 19:39
@quartzmo quartzmo merged commit defdded into googleapis:main Mar 24, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants