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: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

Merged
merged 4 commits into from Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 31 additions & 16 deletions gcp/observability/config.go
Expand Up @@ -20,7 +20,9 @@ package observability

import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
"os"
"regexp"

Expand All @@ -31,9 +33,10 @@ import (
)

const (
envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY"
envProjectID = "GOOGLE_CLOUD_PROJECT"
logFilterPatternRegexpStr = `^([\w./]+)/((?:\w+)|[*])$`
envObservabilityConfig = "GRPC_CONFIG_OBSERVABILITY"
envObservabilityConfigJSON = "GRPC_CONFIG_OBSERVABILITY_JSON"
envProjectID = "GOOGLE_CLOUD_PROJECT"
logFilterPatternRegexpStr = `^([\w./]+)/((?:\w+)|[*])$`
)

var logFilterPatternRegexp = regexp.MustCompile(logFilterPatternRegexpStr)
Expand Down Expand Up @@ -72,21 +75,33 @@ func validateFilters(config *configpb.ObservabilityConfig) error {
return nil
}

// unmarshalAndVerifyConfig unmarshals a json string representing an
// observability config into it's protobuf format, and also verifies the
Copy link
Member

Choose a reason for hiding this comment

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

*its. ' != possession

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched.

// configuration's fields for validity.
func unmarshalAndVerifyConfig(rawJSON json.RawMessage) (*configpb.ObservabilityConfig, error) {
var config configpb.ObservabilityConfig
if err := protojson.Unmarshal(rawJSON, &config); err != nil {
return nil, fmt.Errorf("error parsing observability config: %v", err)
}
if err := validateFilters(&config); err != nil {
return nil, fmt.Errorf("error parsing observability config: %v", err)
}
if config.GlobalTraceSamplingRate > 1 || config.GlobalTraceSamplingRate < 0 {
return nil, fmt.Errorf("error parsing observability config: invalid global trace sampling rate %v", config.GlobalTraceSamplingRate)
}
logger.Infof("Parsed ObservabilityConfig: %+v", &config)
Comment on lines +86 to +92
Copy link
Member

Choose a reason for hiding this comment

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

None of this is technically unmarshalling. Move this out and make a different function? Or name this unmarshalAndVerifyConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I like second option. The reason it was pulled into a helper was needed twice for both env vars, and I don't want to add another helper function/callsite.

return &config, nil
}

func parseObservabilityConfig() (*configpb.ObservabilityConfig, error) {
// Parse the config from ENV var
if content := os.Getenv(envObservabilityConfig); content != "" {
var config configpb.ObservabilityConfig
if err := protojson.Unmarshal([]byte(content), &config); err != nil {
return nil, fmt.Errorf("error parsing observability config from env %v: %v", envObservabilityConfig, err)
}
if err := validateFilters(&config); err != nil {
return nil, fmt.Errorf("error parsing observability config: %v", err)
}
if config.GlobalTraceSamplingRate > 1 || config.GlobalTraceSamplingRate < 0 {
return nil, fmt.Errorf("error parsing observability config: invalid global trace sampling rate %v", config.GlobalTraceSamplingRate)
if fileSystemPath := os.Getenv(envObservabilityConfigJSON); fileSystemPath != "" {
content, err := ioutil.ReadFile(fileSystemPath) // TODO: Switch to os.ReadFile once dropped support for go 1.15
if err != nil {
return nil, fmt.Errorf("error reading observability configuration file %q: %v", fileSystemPath, err)
}
logger.Infof("Parsed ObservabilityConfig: %+v", &config)
return &config, nil
return unmarshalAndVerifyConfig(content)
} else if content := os.Getenv(envObservabilityConfig); content != "" {
return unmarshalAndVerifyConfig([]byte(content))
}
// If the ENV var doesn't exist, do nothing
return nil, nil
Expand Down
89 changes: 89 additions & 0 deletions gcp/observability/observability_test.go
Expand Up @@ -21,7 +21,9 @@ package observability
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io/ioutil"
"net"
"os"
"sync"
Expand Down Expand Up @@ -675,14 +677,101 @@ func (s) TestRefuseStartWithInvalidPatterns(t *testing.T) {
if err != nil {
t.Fatalf("failed to convert config to JSON: %v", err)
}
os.Setenv(envObservabilityConfigJSON, "")
os.Setenv(envObservabilityConfig, string(configJSON))
// If there is at least one invalid pattern, which should not be silently tolerated.
if err := Start(context.Background()); err == nil {
t.Fatalf("Invalid patterns not triggering error")
}
}

// createTmpConfigInFileSystem creates a random observability config at a random
// place in the temporary portion of the file system dependent on system. It
// also sets the environment variable GRPC_CONFIG_OBSERVABILITY_JSON to point to
// this created config.
func createTmpConfigInFileSystem(rawJSON json.RawMessage) (*os.File, error) {
configJSONFile, err := ioutil.TempFile(os.TempDir(), "configJSON-")
if err != nil {
return nil, fmt.Errorf("cannot create file %v: %v", configJSONFile.Name(), err)
}
defer configJSONFile.Close()
_, err = configJSONFile.Write(rawJSON)
if err != nil {
return nil, fmt.Errorf("cannot write marshalled JSON: %v", err)
}
os.Setenv(envObservabilityConfigJSON, configJSONFile.Name())
return configJSONFile, nil /// do you even need to return this file if you're already closing it here?
Copy link
Member

Choose a reason for hiding this comment

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

Should we delete it after the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point. Stored this returned in a local variable at call sites and defered the Close().

}

// TestJSONEnvVarSet tests a valid observability configuration specified by the
// GRPC_CONFIG_OBSERVABILITY_JSON environment variable, whose value represents a
// file path pointing to a JSON encoded config.
func (s) TestJSONEnvVarSet(t *testing.T) {
configJSON := json.RawMessage(`{
Copy link
Member

Choose a reason for hiding this comment

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

Pass to createTmpConfigFile as a string instead of a json.RawMessage to avoid the cast at every callsite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched.

"destinationProjectId": "fake",
"logFilters":[{"pattern":"*","headerBytes":1073741824,"messageBytes":1073741824}]
}`)
_, err := createTmpConfigInFileSystem(configJSON)
if err != nil {
t.Fatalf("failed to create config in file system: %v", err)
}
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
if err := Start(ctx); err != nil {
t.Fatalf("error starting observability with valid config through file system: %v", err)
}
defer End()
}

// TestBothConfigEnvVarsSet tests the scenario where both configuration
// environment variables are set. The file system environment variable should
// take precedence, and an error should return in the case of the file system
// configuration being invalid, even if the direct configuration environment
// variable is set and valid.
func (s) TestBothConfigEnvVarsSet(t *testing.T) {
configJSON := json.RawMessage(`{
"destinationProjectId":"fake",
"logFilters":[{"pattern":":-)"}, {"pattern":"*"}]
}`)
_, err := createTmpConfigInFileSystem(configJSON)
if err != nil {
t.Fatalf("failed to create config in file system: %v", err)
}
// This configuration should be ignored, as precedence 2.
validConfig := &configpb.ObservabilityConfig{
EnableCloudLogging: true,
DestinationProjectId: "fake",
LogFilters: []*configpb.ObservabilityConfig_LogFilter{
{
Pattern: "*",
HeaderBytes: infinitySizeBytes,
MessageBytes: infinitySizeBytes,
},
},
}
validConfigJSON, err := protojson.Marshal(validConfig)
if err != nil {
t.Fatalf("failed to convert config to JSON: %v", err)
}
os.Setenv(envObservabilityConfig, string(validConfigJSON))
if err := Start(context.Background()); err == nil {
t.Fatalf("Invalid patterns not triggering error")
}
}

// TestErrInFileSystemEnvVar tests the scenario where an observability
// configuration is specified with environment variable that specifies a
// location in the file system for configuration, and this location doesn't have
// a file (or valid configuration).
func (s) TestErrInFileSystemEnvVar(t *testing.T) {
os.Setenv(envObservabilityConfigJSON, "/this-file/does-not-exist")
if err := Start(context.Background()); err == nil {
t.Fatalf("Invalid file system path not triggering error")
}
}

func (s) TestNoEnvSet(t *testing.T) {
os.Setenv(envObservabilityConfigJSON, "")
os.Setenv(envObservabilityConfig, "")
// If there is no observability config set at all, the Start should return an error.
if err := Start(context.Background()); err == nil {
Expand Down