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
Merged

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 Feb 16, 2022
@lidizheng
Copy link
Contributor Author

@lidizheng lidizheng commented Feb 16, 2022

@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 requested a review from menghanl 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

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

@lidizheng lidizheng commented Mar 2, 2022

@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

@lidizheng lidizheng commented Mar 4, 2022

@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

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

@lidizheng lidizheng commented Mar 10, 2022

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

@lidizheng
Copy link
Contributor Author

@lidizheng lidizheng commented Mar 15, 2022

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
Copy link
Contributor

@dfawley dfawley commented Apr 5, 2022

Let me know if you decided about the name change. The brutal-force way is changing it to gcpobservability.

We agreed on grpc/gcp/observability for this for now. Java has io.grpc.gcp.observability.

dfawley
dfawley approved these changes Apr 5, 2022
@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
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants