Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(internal/trace): add OpenTelemetry support #8655

Merged
merged 14 commits into from Nov 10, 2023

Conversation

quartzmo
Copy link
Member

@quartzmo quartzmo commented Oct 6, 2023

Add GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING env var for opt-in OpenTelemetry tracing.

refs: #2205

OpenCensus sunsetting and OpenTelemetry support

The OpenCensus project is obsolete and was archived on July 31st, 2023. This means that any security vulnerabilities that are found will not be patched. We recommend that you begin migrating to OpenCensus tracing to OpenTelemetry, the successor project.

One year after the Google Cloud Go client libraries release the experimental, opt-in OpenTelemetry tracing support in this PR (mid-Q4 2023), the prior experimental OpenCensus tracing will be removed.

Using the OpenTelemetry-Go - OpenCensus Bridge, you can immediately begin exporting your traces with OpenTelemetry, even while dependencies remain instrumented with OpenCensus. If you do not use the bridge, you will need to migrate your entire application and all of its instrumented dependencies at once. For simple applications, this may be possible, but we expect the bridge to be helpful if multiple libraries with instrumentation are used.

Please refer to the following resources:

Testing support for library maintainers

PTAL at internal/testutil/trace_otel.go and the update to trace_test.go. I wasn't able to match the existing testutil api perfectly, so testing support for OpenTelemetry will not be completely transparent. However, the new testutil functions are very close:

OpenCensus

te := testutil.NewTestExporter()
defer te.Unregister()
...
spans := te.Spans

OpenTelemetry

ctx := context.Background()
te := testutil.NewOpenTelemetryTestExporter()
defer te.Unregister(ctx)
...
spans := te.Spans()

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Oct 6, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Oct 23, 2023
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Oct 24, 2023
Add GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING env var flag
for opt-in OpenTelemetry tracing.

refs: googleapis#2205
@quartzmo quartzmo marked this pull request as ready for review October 27, 2023 22:11
@quartzmo quartzmo requested a review from a team as a code owner October 27, 2023 22:11
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Nov 6, 2023
internal/trace/trace.go Outdated Show resolved Hide resolved
@@ -53,6 +113,18 @@ func toStatus(err error) trace.Status {
}
}

// toOpenTelemetryStatus converts an error to an equivalent OpenTelemetry status description.
func toOpenTelemetryStatusDescription(err error) string {
var err2 *googleapi.Error
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be an APIError now? Everything should return those now, right? Same with the above toStatus stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into this, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this logic in bigtable shows a situation where the err closing the span could be an unwrapped Status or even a io.EOF:

https://github.com/googleapis/google-cloud-go/blob/bigtable/v1.20.0/bigtable/bigtable.go#L182-L215

Copy link
Member

Choose a reason for hiding this comment

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

sure, but I just meant L117 here. Other cases can still be left.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah sorry, i misunderstood

Copy link
Member Author

Choose a reason for hiding this comment

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

After trying this, I think it's best to just extract the googleapi.Error (if found), since it exposes the HTTP error message. With APIError, that message is only exposed by Error() but it gets some added prefix:

 trace_test.go:131: got googleapi: Error 400: INVALID ARGUMENT, want INVALID ARGUMENT

Copy link
Member

Choose a reason for hiding this comment

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

I think that would be fine to call Error and all HTTP APIError *should have wrap a googleapis.Error. But I am fine with this for now. We can always make that change later.

internal/trace/trace.go Outdated Show resolved Hide resolved
internal/trace/trace_test.go Outdated Show resolved Hide resolved
@quartzmo quartzmo removed the stale: old Pull request is old and needs attention. label Nov 7, 2023
@bhshkh bhshkh marked this pull request as draft November 7, 2023 21:39
@bhshkh bhshkh marked this pull request as ready for review November 7, 2023 21:40
@bhshkh
Copy link
Contributor

bhshkh commented Nov 7, 2023

Sorry, marked as draft by mistake

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Nov 8, 2023
@quartzmo
Copy link
Member Author

I tested this manually using only an OpenTelemetry exporter and the cloud.google.com/go/storage client, and observed the storage span in Cloud Trace console when GOOGLE_API_GO_EXPERIMENTAL_TELEMETRY_PLATFORM_TRACING=opentelemetry.

/cc @cjcotter

@quartzmo quartzmo merged commit 7a46b54 into googleapis:main Nov 10, 2023
9 checks passed
@quartzmo quartzmo deleted the trace-otel branch November 10, 2023 19:01
quartzmo added a commit to quartzmo/google-cloud-go that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large. stale: old Pull request is old and needs attention.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants