Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(internal/trace): add OpenTelemetry support #8655
Changes from 2 commits
4cef291
a66581c
f3ec6f9
377e01e
b557636
3e86c4e
df3bf45
7e634e4
1c8f42e
c04a801
f98c2e5
68cea9a
541e0ee
d0d3438
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should this just be an APIError now? Everything should return those now, right? Same with the above toStatus stuff
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.
I'll look into this, thanks!
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.
I think this logic in bigtable shows a situation where the
err
closing the span could be an unwrappedStatus
or even aio.EOF
:https://github.com/googleapis/google-cloud-go/blob/bigtable/v1.20.0/bigtable/bigtable.go#L182-L215
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.
sure, but I just meant L117 here. Other cases can still be left.
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.
ah sorry, i misunderstood
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.
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 byError()
but it gets some added prefix: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.
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.