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

Conversation

zasweq
Copy link
Contributor

@zasweq zasweq commented Jul 20, 2022

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

@zasweq zasweq requested review from dfawley and easwars July 20, 2022 01:11
@zasweq zasweq added this to the 1.49 Release milestone Jul 20, 2022
@dfawley 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
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +84 to +90
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)
Copy link
Contributor

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.

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Switched to your defined error string.

Comment on lines 690 to 700
config := &configpb.ObservabilityConfig{
EnableCloudLogging: true,
DestinationProjectId: "fake",
LogFilters: []*configpb.ObservabilityConfig_LogFilter{
{
Pattern: "*",
HeaderBytes: infinitySizeBytes,
MessageBytes: infinitySizeBytes,
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

t.Fatalf("failed to convert config to JSON: %v", err)
}

configJSONFile, err := os.Create("/tmp/configJSON")
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).


// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

and _an_ error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah whoops. Switched.

Comment on lines 730 to 741
invalidConfig := &configpb.ObservabilityConfig{
EnableCloudLogging: true,
DestinationProjectId: "fake",
LogFilters: []*configpb.ObservabilityConfig_LogFilter{
{
Pattern: ":-)",
},
{
Pattern: "*",
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

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 to a raw JSON string.

}
os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON")

if err := Start(context.Background()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a context with a timeout?

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.

@dfawley dfawley assigned zasweq and unassigned easwars and dfawley Jul 20, 2022
@dfawley dfawley removed the request for review from easwars July 20, 2022 15:58
@dfawley
Copy link
Contributor

dfawley commented Jul 20, 2022

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

Copy link
Contributor Author

@zasweq zasweq left a 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 :D!

Comment on lines +84 to +90
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)
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.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Switched to your defined error string.


// 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah whoops. Switched.

}
os.Setenv(envObservabilityConfigJSON, "/tmp/configJSON")

if err := Start(context.Background()); err != nil {
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.

t.Fatalf("failed to convert config to JSON: %v", err)
}

configJSONFile, err := os.Create("/tmp/configJSON")
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).

Comment on lines 690 to 700
config := &configpb.ObservabilityConfig{
EnableCloudLogging: true,
DestinationProjectId: "fake",
LogFilters: []*configpb.ObservabilityConfig_LogFilter{
{
Pattern: "*",
HeaderBytes: infinitySizeBytes,
MessageBytes: infinitySizeBytes,
},
},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 730 to 741
invalidConfig := &configpb.ObservabilityConfig{
EnableCloudLogging: true,
DestinationProjectId: "fake",
LogFilters: []*configpb.ObservabilityConfig_LogFilter{
{
Pattern: ":-)",
},
{
Pattern: "*",
},
},
}
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 to a raw JSON string.

// 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
Copy link
Contributor

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.

}
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?
Copy link
Contributor

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().

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

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.

@zasweq zasweq merged commit 679138d into grpc:master Jul 20, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants