-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(pubsub): add otel tracing with links #9594
feat(pubsub): add otel tracing with links #9594
Conversation
…sub-otel-trace-receive-clean
…/google-cloud-go into pubsub-otel-trace-receive-clean
revert lang version changes to go.mod/work revert changes to go.mod fix go otel dependency versions remove unneeded changes revert renaming of ctx add clarifying comment to activeSpans map run go mod tidy commit go.sum run go mod tidy
0256304
to
454e69e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks pretty good :) I need to catch up now...
pubsub/service.go
Outdated
s := strings.Split(fqn, "/") | ||
// Some tests don't use FQN, in which case return empty projectID. | ||
if len(s) == 1 { | ||
return "", s[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a default project retained on the client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for tests, since we're instantiating the iterator
directly via newMessageIterator We should probably change the tests to use an actual project. If that's preferred, I can make that change and remove the check here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If empty project for tests is a consistent idiom for pubsub it's fine for the time being, but might be good to revisit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only happens once, admittedly, so not exactly an idiom. I fixed that test now, but also made this return empty strings for both if the format is not "projects/p/subscriptions/s"
pubsub/service.go
Outdated
s := strings.Split(fqn, "/") | ||
// Some tests don't use FQN, in which case return empty projectID. | ||
if len(s) == 1 { | ||
return "", s[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If empty project for tests is a consistent idiom for pubsub it's fine for the time being, but might be good to revisit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this. Most of my feedback is derived from my less frequent interactions with this client, and the PR itself includes testing and benchmarking for the status quo.
@hongalex can we get another pre-release of this for testing? |
10826f0
into
googleapis:pubsub-otel-trace
Yeah, I created another release:
This doesn't contain changes for configuring num callbacks, but I'll update that branch to pull in changes from here as well. |
Thanks!
That's okay for now, we have another application where we can test the traces without needing to configure num callbacks. |
This replace #8592 and adds the missing subscribe side spans + updates the publish side spans to match our most recent design
Note: this PR uses an experimental golang.org/x/exp/slices package, which was added in go 1.21. This also depends on OpenTelemetry v1.28.0, which requires go 1.21 or higher. This PR will have failing
vet.sh
checks, but should be merged into the base branch,pubsub-otel-trace
. That branch instead should wait on bumping the min go version before merging.