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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ package observability | |
|
||
import ( | ||
"context" | ||
"encoding/json" | ||
"fmt" | ||
"os" | ||
"regexp" | ||
|
@@ -31,9 +32,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) | ||
|
@@ -72,21 +74,32 @@ func validateFilters(config *configpb.ObservabilityConfig) error { | |
return nil | ||
} | ||
|
||
// unmarshalConfig unmarshals a json string representing an observability config | ||
// into it's protobuf format. | ||
func unmarshalConfig(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) | ||
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 := os.ReadFile(fileSystemPath) | ||
if err != nil { | ||
return nil, fmt.Errorf("error reading file from env var %v: %v", envObservabilityConfigJSON, err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Print "error reading observability configuration file %q: %v", fileSystemPath, err" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted. Switched to your defined error string. |
||
} | ||
logger.Infof("Parsed ObservabilityConfig: %+v", &config) | ||
return &config, nil | ||
return unmarshalConfig(content) | ||
} else if content := os.Getenv(envObservabilityConfig); content != "" { | ||
return unmarshalConfig([]byte(content)) | ||
} | ||
// If the ENV var doesn't exist, do nothing | ||
return nil, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -675,14 +675,121 @@ 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") | ||
} | ||
} | ||
|
||
// 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) { | ||
config := &configpb.ObservabilityConfig{ | ||
EnableCloudLogging: true, | ||
DestinationProjectId: "fake", | ||
LogFilters: []*configpb.ObservabilityConfig_LogFilter{ | ||
{ | ||
Pattern: "*", | ||
HeaderBytes: infinitySizeBytes, | ||
MessageBytes: infinitySizeBytes, | ||
}, | ||
}, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: it would be nicer to test this using a raw JSON string instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
configJSON, err := protojson.Marshal(config) | ||
if err != nil { | ||
t.Fatalf("failed to convert config to JSON: %v", err) | ||
} | ||
|
||
configJSONFile, err := os.Create("/tmp/configJSON") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a function that creates a random, non-existent temp file rather than potentially overwriting an existing one owned by the user. Also that allows running multiple times concurrently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion. Done (created a helper function for both tests to call). |
||
if err != nil { | ||
t.Fatalf("cannot create file /tmp/configJSON: %v", err) | ||
} | ||
defer configJSONFile.Close() | ||
_, err = configJSONFile.Write(configJSON) | ||
if err != nil { | ||
t.Fatalf("cannot write marshalled JSON: %v", err) | ||
} | ||
os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON") | ||
|
||
if err := Start(context.Background()); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a context with a timeout? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched. |
||
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 and error should return in the case of the file system | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah whoops. Switched. |
||
// configuration being invalid, even if the direct configuration environment | ||
// variable is set and valid. | ||
func (s) TestBothConfigEnvVarsSet(t *testing.T) { | ||
invalidConfig := &configpb.ObservabilityConfig{ | ||
EnableCloudLogging: true, | ||
DestinationProjectId: "fake", | ||
LogFilters: []*configpb.ObservabilityConfig_LogFilter{ | ||
{ | ||
Pattern: ":-)", | ||
}, | ||
{ | ||
Pattern: "*", | ||
}, | ||
}, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched to a raw JSON string. |
||
invalidConfigJSON, err := protojson.Marshal(invalidConfig) | ||
if err != nil { | ||
t.Fatalf("failed to convert config to JSON: %v", err) | ||
} | ||
|
||
invalidConfigJSONFile, err := os.Create("/tmp/InvalidConfigJSON") | ||
if err != nil { | ||
t.Fatalf("cannot create file /tmp/InvalidConfigJSON: %v", err) | ||
} | ||
defer invalidConfigJSONFile.Close() | ||
_, err = invalidConfigJSONFile.Write(invalidConfigJSON) | ||
if err != nil { | ||
t.Fatalf("cannot write marshalled JSON: %v", err) | ||
} | ||
os.Setenv(envObservabilityConfigJSON, "/tmp/InvalidConfigJSON") | ||
|
||
// 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 { | ||
|
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.
None of this is technically unmarshalling. Move this out and make a different function? Or name this
unmarshalAndVerifyConfig
?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.
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.