-
Notifications
You must be signed in to change notification settings - Fork 4.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
gcp/observability: implement public preview config syntax, logging schema, and exposed metrics #5704
Conversation
2356e03
to
8882810
Compare
8882810
to
9b7e76d
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.
Mostly nits so far, but I haven't dug into the tests.
We need to get this running in an integration type of test ASAP to make sure we figure out if it's using the proper JSON field names for the config and log entries being exported.
if method == "*" { | ||
if exclude { | ||
return errors.New("cannot have exclude and a '*' wildcard") | ||
} | ||
continue |
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.
Was there also supposed to be an error if "*"
isn't last in the list?
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.
Ping on this one.
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.
Oh sorry forgot to respond to this earlier (I swear I typed something out at least - it must've gotten lost). No, there was not supposed to be an error here. Previously, yes this was speced out somewhere (was a docstring in the config type by Lidi). However, now we decided not to do any preprocessing (me and you had a discussion about this) and left it up to the user to know if he specifies a * somewhere in method[] for an event, no further event will hit, and entrusted the user with more responsibility in that regard, and trusted the user.
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.
Thanks for the comments boss man :D!
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.
Almost fully LGTM. Mostly just some small things.
Please also add a test that configures payload and metadata truncation and verifies the output events.
gcp/observability/config.go
Outdated
envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY" | ||
envObservabilityConfigJSON = "GRPC_CONFIG_OBSERVABILITY_JSON" |
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.
The only env vars we have now are GRPC_OBSERVABILITY_CONFIG
and GRPC_OBSERVABILITY_CONFIG_FILE
.
Can we add these into envconfig instead?
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.
Whoops, didn't realize there was a change in the env var names. Thanks for finding this. Sorry, looking at internal/envconfig, it doesn't seem like the correct place for these. One of the files is for grpc settings, one of the files is for xDS. But since you suggested it, I'll reluctantly add it there. I added a new file called observability, with a similar structure to the file xds.go in that package.
gcp/observability/logging_test.go
Outdated
func cmpLoggingEntryList(entryList1 []*grpcLogEntry, entryList2 []*grpcLogEntry) error { | ||
if diff := cmp.Diff(entryList1, entryList2, |
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.
Please name these got
and want
so it's clear how to call it when adding a new test case.
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.
Done.
gcp/observability/logging_test.go
Outdated
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { |
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.
Send a message here to make sure ServerMessage
works for streaming RPCs?
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 send.
gcp/observability/logging_test.go
Outdated
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { |
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.
As above; send a message?
Also it seems worthwhile to send a message from the client in both cases.
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.
Done. Added send from client in both cases.
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.
Also, something I just realized is this takes away having to change this test in the future due to that bin logging bug of logging Server Headers on Trailers only response. Luckily we have it clearly documented in other PRs test and this PRs history.
gcp/observability/logging_test.go
Outdated
grpcLogEntriesWant := make([]*grpcLogEntry, 5) | ||
grpcLogEntriesWant[0] = &grpcLogEntry{ |
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.
FWIW you can initialize these like:
grpcLogEntriesWant := []*grpcLogEntry{
{
Type: eventTypeClientHeader,
Logger: ....
...
}, {
Type: eventTypeClientMessage,
...
}
}
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.
Yeah, this way is much cleaner. Thanks. Switched.
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.
Thanks for the pass bossman :D! Got to all your comments and added a headers/message truncated test.
gcp/observability/logging_test.go
Outdated
func cmpLoggingEntryList(entryList1 []*grpcLogEntry, entryList2 []*grpcLogEntry) error { | ||
if diff := cmp.Diff(entryList1, entryList2, |
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.
Done.
gcp/observability/config.go
Outdated
envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY" | ||
envObservabilityConfigJSON = "GRPC_CONFIG_OBSERVABILITY_JSON" |
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.
Whoops, didn't realize there was a change in the env var names. Thanks for finding this. Sorry, looking at internal/envconfig, it doesn't seem like the correct place for these. One of the files is for grpc settings, one of the files is for xDS. But since you suggested it, I'll reluctantly add it there. I added a new file called observability, with a similar structure to the file xds.go in that package.
gcp/observability/logging_test.go
Outdated
grpcLogEntriesWant := make([]*grpcLogEntry, 5) | ||
grpcLogEntriesWant[0] = &grpcLogEntry{ |
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.
Yeah, this way is much cleaner. Thanks. Switched.
gcp/observability/logging_test.go
Outdated
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { |
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 send.
gcp/observability/logging_test.go
Outdated
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { |
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.
Done. Added send from client in both cases.
gcp/observability/logging_test.go
Outdated
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { |
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.
Also, something I just realized is this takes away having to change this test in the future due to that bin logging bug of logging Server Headers on Trailers only response. Luckily we have it clearly documented in other PRs test and this PRs history.
036951c
to
1c60786
Compare
gcp/observability/config.go
Outdated
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | ||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | ||
if envconfig.ObservabilityConfigFile != "" { | ||
content, err := ioutil.ReadFile(envconfig.ObservabilityConfigFile) // TODO: Switch to os.ReadFile once dropped support for go 1.15 |
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.
Consider:
if envconfig.ObservabilityConfig != "" {
grpclog.Warning("Ignoring GRPC_OBSERVABILITY_CONFIG and using GRPC_OBSERVABILITY_CONFIG_FILE contents.")
}
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.
Done, except with GCP added.
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.
Also I'm assuming you wanted this call on the logger, not the package grpclog? Used the logger.
gcp/observability/config.go
Outdated
@@ -113,14 +112,14 @@ func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*config, error) { | |||
} | |||
|
|||
func parseObservabilityConfig() (*config, error) { | |||
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | |||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | |||
if envconfig.ObservabilityConfigFile != "" { |
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.
To avoid repeating this long exported variable you can do:
if f := envconfig.ObservabilityConfigFile; f != "" {
// use f
}
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 thing. Switched. I was thinking of this too but didn't know if the assignment to a local var (copying to a string on the stack) was worth the code simplification it provides.
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.
The else if below the "long exported variable" is used once so I think it's fair there.
gcp/observability/logging_test.go
Outdated
@@ -131,8 +133,13 @@ func (s) TestClientRPCEventsLogAll(t *testing.T) { | |||
}, | |||
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | |||
for { |
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 don't think this actually wants to be in a loop. The client just sends once, right?
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, lol fair point :). Lol unnecessary while true.
gcp/observability/logging_test.go
Outdated
@@ -306,8 +341,13 @@ func (s) TestServerRPCEventsLogAll(t *testing.T) { | |||
}, | |||
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | |||
for { |
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.
Similar re: not using a loop 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.
Got rid of every while true loop in all my FullDuplexCallF definitions.
gcp/observability/logging_test.go
Outdated
@@ -671,7 +671,7 @@ func (s) TestClientRPCEventsTruncateHeaderAndMetadata(t *testing.T) { | |||
Authority: ss.Address, | |||
SequenceID: 1, | |||
Payload: payload{ | |||
Metadata: map[string]string{"key1": "value1"}, | |||
Metadata: map[string]string{}, |
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.
This is definitely unfortunate that we don't get anything here. It would be better if we could accept either header. There should be a way to do that in the cmp
package.
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.
Ok, I looked through cmp/cmpopts, and I couldn't find an explicit option that had the behavior you desire. The solution I came up with is to ignore the metadata in the cmp.Diff, and then compare with an or explicitly afterward.
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.
Thanks for the comments bossman :D!
gcp/observability/config.go
Outdated
@@ -113,14 +112,14 @@ func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*config, error) { | |||
} | |||
|
|||
func parseObservabilityConfig() (*config, error) { | |||
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | |||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | |||
if envconfig.ObservabilityConfigFile != "" { |
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 thing. Switched. I was thinking of this too but didn't know if the assignment to a local var (copying to a string on the stack) was worth the code simplification it provides.
gcp/observability/config.go
Outdated
@@ -113,14 +112,14 @@ func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*config, error) { | |||
} | |||
|
|||
func parseObservabilityConfig() (*config, error) { | |||
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | |||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | |||
if envconfig.ObservabilityConfigFile != "" { |
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.
The else if below the "long exported variable" is used once so I think it's fair there.
gcp/observability/config.go
Outdated
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | ||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | ||
if envconfig.ObservabilityConfigFile != "" { | ||
content, err := ioutil.ReadFile(envconfig.ObservabilityConfigFile) // TODO: Switch to os.ReadFile once dropped support for go 1.15 |
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.
Done, except with GCP added.
gcp/observability/logging_test.go
Outdated
@@ -131,8 +133,13 @@ func (s) TestClientRPCEventsLogAll(t *testing.T) { | |||
}, | |||
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | |||
for { |
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, lol fair point :). Lol unnecessary while true.
gcp/observability/logging_test.go
Outdated
@@ -306,8 +341,13 @@ func (s) TestServerRPCEventsLogAll(t *testing.T) { | |||
}, | |||
FullDuplexCallF: func(stream grpc_testing.TestService_FullDuplexCallServer) error { | |||
for { |
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.
Got rid of every while true loop in all my FullDuplexCallF definitions.
gcp/observability/logging_test.go
Outdated
@@ -671,7 +671,7 @@ func (s) TestClientRPCEventsTruncateHeaderAndMetadata(t *testing.T) { | |||
Authority: ss.Address, | |||
SequenceID: 1, | |||
Payload: payload{ | |||
Metadata: map[string]string{"key1": "value1"}, | |||
Metadata: map[string]string{}, |
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.
Ok, I looked through cmp/cmpopts, and I couldn't find an explicit option that had the behavior you desire. The solution I came up with is to ignore the metadata in the cmp.Diff, and then compare with an or explicitly afterward.
gcp/observability/config.go
Outdated
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" { | ||
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | ||
if envconfig.ObservabilityConfigFile != "" { | ||
content, err := ioutil.ReadFile(envconfig.ObservabilityConfigFile) // TODO: Switch to os.ReadFile once dropped support for go 1.15 |
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.
Also I'm assuming you wanted this call on the logger, not the package grpclog? Used the logger.
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.
Please check len() of metadata in the test that is expecting truncation. And fix the other final issue. Otherwise LGTM!
gcp/observability/config.go
Outdated
if envconfig.ObservabilityConfigFile != "" { | ||
content, err := ioutil.ReadFile(envconfig.ObservabilityConfigFile) // TODO: Switch to os.ReadFile once dropped support for go 1.15 | ||
if f := envconfig.ObservabilityConfigFile; f != "" { | ||
logger.Warning("Ignoring GRPC_GCP_OBSERVABILITY_CONFIG and using GRPC_GCP_OBSERVABILITY_CONFIG_FILE contents.") |
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.
You can't log this warning unconditionally; only if the other var is set.
…hema, and exposed metrics (grpc#5704)
…ersion to 1.50.1 (#5722) * Add binary logger option for client and server (#5675) * Add binary logger option for client and server * gcp/observability: implement public preview config syntax, logging schema, and exposed metrics (#5704) * Fix o11y typo (#5719) * o11y: Fixed o11y bug (#5720) * update version to 1.50.1
This PR implements all the changes defined for public preview. The biggest change is the configuration change, which switched the logging from precedence based to iterative based and added a partition between client and server RPC events. The second one is the logging schema change defined by Eric, which I used an internal go struct to represent. The final trivial change is the change in the metrics collected/views registered to only Completed RPCs.
RELEASE NOTES: