gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

zasweq · 2022-07-20T01:11:08Z

This PR adds support for the Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON, which configures observability through a file at a path specified by the value of this Environment Variable (see private preview 2 document: go/grpc-observability-ug-2). The precedence ordering of this environment variable taking precedence over GRPC_CONFIG_OBSERVABILITY is based on the grpc-java implementation to keep it consistent across languages, and not officially defined in the design document.

RELEASE NOTES:

gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON

dfawley

Looks really good; just some nits (mostly in the tests).

dfawley · 2022-07-20T15:49:05Z

gcp/observability/config.go

+	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)


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

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.

dfawley · 2022-07-20T15:52:00Z

gcp/observability/config.go

+	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)


Print fileSystemPath? Also avoid shorthand/abbreviations in user-visible error messages ("env var" -> "environment variable").

"error reading observability configuration file %q: %v", fileSystemPath, err"

Noted. Switched to your defined error string.

dfawley · 2022-07-20T15:53:01Z

gcp/observability/observability_test.go

+	config := &configpb.ObservabilityConfig{
+		EnableCloudLogging:   true,
+		DestinationProjectId: "fake",
+		LogFilters: []*configpb.ObservabilityConfig_LogFilter{
+			{
+				Pattern:      "*",
+				HeaderBytes:  infinitySizeBytes,
+				MessageBytes: infinitySizeBytes,
+			},
+		},
+	}


Nit: it would be nicer to test this using a raw JSON string instead.

dfawley · 2022-07-20T15:54:04Z

gcp/observability/observability_test.go

+		t.Fatalf("failed to convert config to JSON: %v", err)
+	}
+
+	configJSONFile, err := os.Create("/tmp/configJSON")


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.

Thanks for the suggestion. Done (created a helper function for both tests to call).

dfawley · 2022-07-20T15:55:28Z

gcp/observability/observability_test.go

+
+// 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


and _an_ error

Ah whoops. Switched.

dfawley · 2022-07-20T15:55:55Z

gcp/observability/observability_test.go

+	invalidConfig := &configpb.ObservabilityConfig{
+		EnableCloudLogging:   true,
+		DestinationProjectId: "fake",
+		LogFilters: []*configpb.ObservabilityConfig_LogFilter{
+			{
+				Pattern: ":-)",
+			},
+			{
+				Pattern: "*",
+			},
+		},
+	}


Switched to a raw JSON string.

dfawley · 2022-07-20T15:56:26Z

gcp/observability/observability_test.go

+	}
+	os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON")
+
+	if err := Start(context.Background()); err != nil {


Use a context with a timeout?

dfawley · 2022-07-20T15:59:18Z

BTW the tests are failing:

--- FAIL: Test (0.26s)
    --- FAIL: Test/JSONEnvVarSet (0.06s)
        tlogger.go:116: INFO config.go:90 [observability] Parsed ObservabilityConfig: enable_cloud_logging:true  destination_project_id:"fake"  log_filters:{pattern:"*"  header_bytes:1073741824  message_bytes:1073741824}  (t=+180.511µs)
        observability_test.go:719: error starting observability with valid config through file system: unable to create CloudLogging exporter: failed to create cloudLoggingExporter: google: could not find default credentials. See https://developers.google.com/accounts/docs/application-default-credentials for more information.
    observability_test.go:387: Span[grpc.testing.TestService.UnaryCall]
    observability_test.go:387: Span[grpc.testing.TestService.UnaryCall]
FAIL

zasweq

Thanks for the comments :D!

zasweq · 2022-07-20T17:19:35Z

gcp/observability/config.go

+	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)


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.

zasweq · 2022-07-20T17:27:34Z

gcp/observability/config.go

+	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)


Noted. Switched to your defined error string.

zasweq · 2022-07-20T17:29:25Z

gcp/observability/observability_test.go

+
+// 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


Ah whoops. Switched.

zasweq · 2022-07-20T18:05:39Z

gcp/observability/observability_test.go

+	}
+	os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON")
+
+	if err := Start(context.Background()); err != nil {


zasweq · 2022-07-20T19:49:37Z

gcp/observability/observability_test.go

+		t.Fatalf("failed to convert config to JSON: %v", err)
+	}
+
+	configJSONFile, err := os.Create("/tmp/configJSON")


Thanks for the suggestion. Done (created a helper function for both tests to call).

zasweq · 2022-07-20T19:49:58Z

gcp/observability/observability_test.go

+	config := &configpb.ObservabilityConfig{
+		EnableCloudLogging:   true,
+		DestinationProjectId: "fake",
+		LogFilters: []*configpb.ObservabilityConfig_LogFilter{
+			{
+				Pattern:      "*",
+				HeaderBytes:  infinitySizeBytes,
+				MessageBytes: infinitySizeBytes,
+			},
+		},
+	}


zasweq · 2022-07-20T19:50:16Z

gcp/observability/observability_test.go

+	invalidConfig := &configpb.ObservabilityConfig{
+		EnableCloudLogging:   true,
+		DestinationProjectId: "fake",
+		LogFilters: []*configpb.ObservabilityConfig_LogFilter{
+			{
+				Pattern: ":-)",
+			},
+			{
+				Pattern: "*",
+			},
+		},
+	}


Switched to a raw JSON string.

dfawley · 2022-07-20T20:20:07Z

gcp/observability/config.go

-// into it's protobuf format.
-func unmarshalConfig(rawJSON json.RawMessage) (*configpb.ObservabilityConfig, error) {
+// unmarshalAndVerifyConfig unmarshals a json string representing an
+// observability config into it's protobuf format, and also verifies the


*its. ' != possession

dfawley · 2022-07-20T20:22:09Z

gcp/observability/observability_test.go

 	}
-	os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON")
+	os.Setenv(envObservabilityConfigJSON, configJSONFile.Name())
+	return configJSONFile, nil /// do you even need to return this file if you're already closing it here?


Should we delete it after the test?

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

dfawley · 2022-07-20T20:23:05Z

gcp/observability/observability_test.go

+// 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(`{


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

zasweq requested review from dfawley and easwars July 20, 2022 01:11

zasweq assigned dfawley and easwars Jul 20, 2022

zasweq added the Type: Feature New features or improvements in behavior label Jul 20, 2022

zasweq added this to the 1.49 Release milestone Jul 20, 2022

Added support for Enviornment Variable path specifier

b043e95

zasweq force-pushed the observability branch from abc0189 to b043e95 Compare July 20, 2022 01:15

dfawley changed the title ~~O11y: Added support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON~~ gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON Jul 20, 2022

dfawley reviewed Jul 20, 2022

View reviewed changes

dfawley assigned zasweq and unassigned easwars and dfawley Jul 20, 2022

dfawley removed the request for review from easwars July 20, 2022 15:58

zasweq commented Jul 20, 2022

View reviewed changes

zasweq added 2 commits July 20, 2022 15:53

Responded to Doug's comments

098cbd7

Switch os.ReadFile to ioutil.ReadFile

b7e6f7c

dfawley approved these changes Jul 20, 2022

View reviewed changes

Responded to Doug's comments

84c612c

zasweq merged commit 679138d into grpc:master Jul 20, 2022

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

moul-bot mentioned this pull request Oct 17, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.1 moul/depviz#586

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

zasweq commented Jul 20, 2022 •

edited by dfawley

Loading

dfawley left a comment

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley commented Jul 20, 2022

zasweq left a comment

zasweq Jul 20, 2022

zasweq Jul 20, 2022

zasweq Jul 20, 2022

zasweq Jul 20, 2022

zasweq Jul 20, 2022

zasweq Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

dfawley Jul 20, 2022

zasweq Jul 20, 2022

gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

gcp/observability: Add support for Environment Variable GRPC_CONFIG_OBSERVABILITY_JSON #5525

Conversation

zasweq commented Jul 20, 2022 • edited by dfawley Loading

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Jul 20, 2022

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Jul 20, 2022 •

edited by dfawley

Loading