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

gcp/observability: implement logging via binarylog #5196

Merged
merged 23 commits into from Apr 6, 2022

Conversation

lidizheng
Copy link
Contributor

@lidizheng lidizheng commented Feb 16, 2022

The plugin registers a binarylog Logger and consumes a binarylog Sink directly from the internal API, without any temporary file in between.

The designed usage looks like:

import "google.golang.org/grpc/observability"

func main() {
    if err := observability.Start(context.Background()); err != nil {
        // Handle the error
    }
    defer observability.End()
    // Use gRPC as normal
}

There is still vagueness inside the proto definition (grpc/grpc-proto#103, pending to be formed as gRFC). As discussed before, I hid them as internal module. The translation between binary log entry and GrpcLogRecord is almost 1-to-1, but misses details like:

  • Service name and method name won't present in all events
  • Peer address is not available in client initiated request
  • New proto doesn't have payload length

Previous attempt: #5157

RELEASE NOTES: none

@lidizheng lidizheng marked this pull request as ready for review February 16, 2022 03:34
@lidizheng
Copy link
Contributor Author

@dfawley PTAL.

observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/exporting.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned menghanl and lidizheng and unassigned dfawley Feb 19, 2022
@dfawley dfawley changed the title feat: implement grpc-observability logging via binarylog observability: implement logging via binarylog Feb 19, 2022
@easwars easwars added this to the 1.46 Release milestone Feb 22, 2022
Copy link
Contributor Author

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review!

I addressed all comments (or at least try to come up with a solution).

PTALA.

observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/exporting.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
@lidizheng
Copy link
Contributor Author

@dfawley How's the review going? Feel free to post partial or high-level comments. Let me know how can I push this PR forward.

@lidizheng
Copy link
Contributor Author

@dfawley PTALA. I have updated this PR according to our offline discussion. The data race around binarylog logger is resolved.

observability/config.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/exporting.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review! PTALAAA.

observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/exporting.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/observability.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
observability/logging.go Outdated Show resolved Hide resolved
@lidizheng
Copy link
Contributor Author

@dfawley PTALAAAA. I have lined up 2 child PRs after this #5233, #5234.

@lidizheng
Copy link
Contributor Author

Bump.

observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/config.go Outdated Show resolved Hide resolved
observability/exporting.go Outdated Show resolved Hide resolved
@dfawley dfawley changed the title observability: implement logging via binarylog gcp/observability: implement logging via binarylog Apr 6, 2022
@dfawley dfawley merged commit 4467a29 into grpc:master Apr 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants