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: update observability logging proto #9608
Conversation
gcp-observability/src/main/proto/grpc/observabilitylog/v1/observabilitylog.proto
Show resolved
Hide resolved
CC @ejona86 since he might be interested in reviewing this as well |
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/ObservabilityConfigImpl.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/LogHelper.java
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/LogHelper.java
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/LogHelper.java
Show resolved
Hide resolved
gcp-observability/src/main/proto/grpc/observabilitylog/v1/observabilitylog.proto
Show resolved
Hide resolved
gcp-observability/src/main/proto/grpc/observabilitylog/v1/observabilitylog.proto
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/logging/GcpLogSink.java
Show resolved
Hide resolved
.setAuthority(authority) | ||
.setType(eventType) | ||
.setLogger(eventLogger) | ||
.setPayload(pair.payloadBuilder) |
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.
pair
can be null (line 243-245) in which case you will get NPE?
Before fixing the code, you should create a unit test that reproduces the NPE and then fix the code.
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.
Added a test to reproduce NPE. Added a null check before setting values of GrpcLogRecord
using pair
.
gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/LogHelper.java
Outdated
Show resolved
Hide resolved
gcp-observability/src/main/java/io/grpc/gcp/observability/interceptors/LogHelper.java
Show resolved
Hide resolved
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.
couple of comments need to be addressed
gcp-observability/src/test/java/io/grpc/gcp/observability/interceptors/LogHelperTest.java
Outdated
Show resolved
Hide resolved
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.
Approved but have a comment about using new String("...")
which will be good to fix
This PR includes changes to observability logging data model / proto as per latest logging design review.
Explicit flush is removed to rely on default batching settings provided by Google Cloud Logging Client library.
Ref b/248096772