Skip to content

feat(transport/all): Add ability to specify []otelhttp.Opts for transport when create new HTTP client #3130

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

surik
Copy link

@surik surik commented Apr 30, 2025

See #3138

@surik surik requested a review from a team as a code owner April 30, 2025 11:24
Copy link

google-cla bot commented Apr 30, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@surik surik force-pushed the allow-custom-otel-http-tracing-provider branch 2 times, most recently from 637aa91 to 0ba3fdd Compare April 30, 2025 11:28
@surik surik changed the title Add ability to specify []otelhttp.Opts for transport when create new HTTP client feat(transport/http): Add ability to specify []otelhttp.Opts for transport when create new HTTP client Apr 30, 2025
@quartzmo quartzmo self-assigned this May 1, 2025
@quartzmo
Copy link
Member

quartzmo commented May 1, 2025

@surik Can you please open a feature request issue in this repo explaining your need for otelhttp.Option in the HTTP Client? Thank you.

@surik
Copy link
Author

surik commented May 5, 2025

Hi @quartzmo, I've opened a request #3138

@surik surik force-pushed the allow-custom-otel-http-tracing-provider branch from 048ca08 to 334fb10 Compare May 12, 2025 15:45
@surik surik changed the title feat(transport/http): Add ability to specify []otelhttp.Opts for transport when create new HTTP client feat(transport/all): Add ability to specify []otelhttp.Opts for transport when create new HTTP client May 13, 2025
@surik surik force-pushed the allow-custom-otel-http-tracing-provider branch from 334fb10 to 899af84 Compare May 13, 2025 13:55
quartzmo
quartzmo previously approved these changes May 13, 2025
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2025
@quartzmo
Copy link
Member

Kokoro CI failed with staticcheck:

internal/settings.go:193:64: type assertion to the same type: o already has type otelgrpc.Option (S1040)

@surik surik force-pushed the allow-custom-otel-http-tracing-provider branch from a1ce8e4 to 2c7e275 Compare May 14, 2025 07:23
@quartzmo quartzmo requested a review from codyoss May 14, 2025 16:26
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 14, 2025
@@ -306,7 +306,7 @@ func addOpenTelemetryTransport(trans http.RoundTripper, settings *internal.DialS
if settings.TelemetryDisabled {
return trans
}
return otelhttp.NewTransport(trans)
return otelhttp.NewTransport(trans, settings.OpenTelemetryOptsHTTP...)
Copy link
Member

Choose a reason for hiding this comment

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

For both this and the gRPC package this change is fine to make, but it won't actually enable the feature for most users -- that is because these packages, by default, delegate to our new auth library which has its own transport packages: https://pkg.go.dev/cloud.google.com/go/auth

At the very least I think we should add some TODOs and keep the issue open until this is plumbed all the way through to the new auth packages. See the newClientNewAuth function in this file as an example of how we delegate calls to this new library.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you @codyoss! I added a todo.

@surik surik force-pushed the allow-custom-otel-http-tracing-provider branch from 5c1a701 to 7803b46 Compare May 15, 2025 14:56
@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2025
@quartzmo quartzmo assigned codyoss and unassigned quartzmo May 16, 2025
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.

4 participants