Merge pull request #2113 from carolynvs/remove-structured-logs-exp-flag

Remove structured logs exp flag

CONTRIBUTING.md

@@ -444,7 +444,7 @@ the future. If you don't see your post, change the date to today's date.
 
 ## View a trace of a Porter command
 
-Porter has an experimental feature, structured-logs, that sends trace data about the commands run to an OpenTelemetry backend.
+Porter can send trace data about the commands run to an OpenTelemetry backend.
 It can be very helpful when figuring out why a command failed because you can see the values of variables and stack traces.
 
 In development, you can use the [otel-jaeger bundle] to set up a development instance of Jaeger, which gives you a nice website to see each command run.
@@ -458,15 +458,13 @@ This tells Porter to turn on tracing, and connect to OpenTelemetry server that y
 
 **Posix**
 ```bash
-export PORTER_EXPERIMENTAL="structured-logs"
 export PORTER_TELEMETRY_ENABLED="true"
 export OTEL_EXPORTER_OTLP_PROTOCOL="grpc"
 export OTEL_EXPORTER_OTLP_INSECURE="true"
 ```
 
 **Powershell**
 ```powershell
-$env:PORTER_EXPERIMENTAL="structured-logs"
 $env:PORTER_TELEMETRY_ENABLED="true"
 $env:OTEL_EXPORTER_OTLP_PROTOCOL="grpc"
 $env:OTEL_EXPORTER_OTLP_INSECURE="true"

cmd/porter/main_test.go

@@ -92,22 +92,22 @@ func TestExperimentalFlags(t *testing.T) {
 		cmd := buildRootCommandFrom(p.Porter)
 		cmd.SetArgs([]string{"install"})
 		cmd.Execute()
-		assert.False(t, p.Config.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.False(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
 	t.Run("flag set", func(t *testing.T) {
 		p := porter.NewTestPorter(t)
 		defer p.Close()
 
 		cmd := buildRootCommandFrom(p.Porter)
-		cmd.SetArgs([]string{"install", "--experimental", experimental.StructuredLogs})
+		cmd.SetArgs([]string{"install", "--experimental", experimental.NoopFeature})
 		cmd.Execute()
 
-		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
 	t.Run("flag unset, env set", func(t *testing.T) {
-		os.Setenv(expEnvVar, experimental.StructuredLogs)
+		os.Setenv(expEnvVar, experimental.NoopFeature)
 		defer os.Unsetenv(expEnvVar)
 
 		p := porter.NewTestPorter(t)
@@ -117,6 +117,6 @@ func TestExperimentalFlags(t *testing.T) {
 		cmd.SetArgs([]string{"install"})
 		cmd.Execute()
 
-		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.True(t, p.Config.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 }

docs/content/administrators/diagnostics.md

@@ -3,7 +3,7 @@ title: Collect Diagnostics from Porter
 description: How to configure Porter to generate logs and telemetry data for diagnostic purposes
 ---
 
-When the [structured-logs experimental feature][structured-logs] is enabled, Porter generates two types of data to assist with diagnostics and troubleshooting:
+Porter can generate two types of data to assist with diagnostics and troubleshooting:
 
 * [Logs](#logs)
 * [Telemetry](#telemetry)
@@ -20,15 +20,15 @@ See [Log Settings] for details on how to configure logging.
 ## Telemetry
 
 Porter is compatible with the [OpenTelemetry] specification and generates trace data that can be sent to a [compatible services][compat].
-When the [structured-logs experimental feature][structured-logs] and [telemetry] is enabled, Porter automatically uses the standard [OpenTelemetry environment variables] to configure the trace exporter.
+Porter automatically uses the standard [OpenTelemetry environment variables] to configure the trace exporter.
 
 Below is an example trace from running the porter upgrade command. You can see timings for each part of the command, and relevant variables used.
 
 ![Screen shot of the Jaeger UI showing that porter upgrade was run](/administrators/jaeger-trace-example.png)
 
-If you are running a local grpc OpenTelemetry collector, for example with the [otel-jaeger bundle], you can set the following environment variables to have Porter send telemetry data to it. This turns on the [structured-logs experimental feature][structured-logs], enables telemetry, and uses standard OpenTelemetry environment variables to point to an unsecured grpc OpenTelemetry collector.
+If you are running a local grpc OpenTelemetry collector, for example with the [otel-jaeger bundle], you can set the following environment variables to have Porter send telemetry data to it. 
+The environment variables below enable telemetry, and use standard OpenTelemetry environment variables to point to an unsecured grpc OpenTelemetry collector.
 
-* PORTER_EXPERIMENTAL: structured-logs
 * PORTER_TELEMETRY_ENABLED: true
 * OTEL_EXPORTER_OTLP_PROTOCOL: grpc
 * OTEL_EXPORTER_OTLP_ENDPOINT: 127.0.0.1:4317
@@ -40,6 +40,5 @@ See [Telemetry Settings][telemetry] for all the supported configuration settings
 [OpenTelemetry environment variables]: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.8.0/specification/protocol/exporter.md
 [telemetry]: /configuration/#telemetry
 [Log Settings]: /configuration/#logs
-[structured-logs]: /configuration/#structured-logs
 [OpenTelemetry]: https://opentelemetry.io
 [otel-jaeger bundle]: /examples/src/otel-jaeger

docs/content/configuration.md

@@ -73,8 +73,8 @@ output = "json"
 # Allow all bundles access to the Docker Host
 allow-docker-host-access = true
 
-# Enable experimental v1 features
-experimental = ["build-drivers", "structured-logs"]
+# Enable experimental features
+experimental = ["flagA", "flagB"]
 
 # Use Docker buildkit to build the bundle
 build-driver = "buildkit"
@@ -134,7 +134,11 @@ default-secrets-plugin = "kubernetes.secret"
 # Log command output to a file in PORTER_HOME/logs/
 [logs]
   # Log command output to a file
-  enabled = true
+  log-to-file = true
+
+  # When structured is true, the logs printed to the console 
+  # include a timestamp and log level
+  structured = false
 
   # Sets the log level for what is written to the file
   # Allowed values: debug, info, warn, error
@@ -191,32 +195,29 @@ feature by:
 
 ### Build Drivers
 
-The **build-drivers** experimental feature flag is no longer used.
+The **build-drivers** experimental feature flag is no longer active.
 Build drivers are enabled by default and the only available driver is buildkit.
 
 ### Structured Logs
 
-The **structured-logs** experimental feature flag enables advanced [logging configuration](#logs)
-and exporting [telemetry](#telemetry) data.
-
-When this feature is enabled, the logs output to the console will contain additional information such as the log level, timestamp and optional context information.
+The **structured-logs** experimental feature flag is no longer active.
+Use the trace and logs configuration sections below to configure how logs and telemetry should be collected.
 
 #### Logs
 
 Porter can be configured to [write a logfile for each command](/administrators/diagnostics/#logs).
-This feature requires the [structured-logs](#structured-logs) feature to be enabled.
 
 The following log settings are available:
 
-| Setting | Environment Variable | Description |
-| -------------- | -------------------- | ----------- |
-| logs.enabled | PORTER_LOGS_ENABLED | Specifies if a logfile should be written for each command. |
-| logs.level | PORTER_LOGS_LEVEL | Filters the logs to the specified level and higher. The log level controls both the logs written to file, and the logs output to the console when porter is run. Allowed values are: debug, info, warn, error. |
+| Setting          | Environment Variable    | Description                                                                                                                                                                                                    |
+|------------------|-------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| logs.log-to-file | PORTER_LOGS_LOG_TO_FILE | Specifies if a logfile should be written for each command.                                                                                                                                                     |
+| logs.structured  | PORTER_LOGS_STRUCTURED  | Specifies if the logs printed to the console should include a timestamp and log level                                                                                                                          | 
+| logs.level       | PORTER_LOGS_LEVEL       | Filters the logs to the specified level and higher. The log level controls both the logs written to file, and the logs output to the console when porter is run. Allowed values are: debug, info, warn, error. |
 
 #### Telemetry
 
 Porter supports the OpenTelemetry specification for exporting trace data.
-This feature requires the [structured-logs](#structured-logs) feature to be enabled.
 
 Porter automatically uses the standard [OpenTelemetry environment variables][otel] to configure the trace exporter.
 
@@ -234,8 +235,6 @@ Porter automatically uses the standard [OpenTelemetry environment variables][ote
 Below is a sample Porter configuration file that demonstrates how to set each of the telemetry settings:
 
 ```toml
-experimental = ["structured-logs"]
-
 [telemetry]
   enabled = true
   protocol = "grpc"

pkg/build/buildkit/buildx.go

@@ -13,7 +13,6 @@ import (
 	"get.porter.sh/porter/pkg/build"
 	"get.porter.sh/porter/pkg/cnab"
 	"get.porter.sh/porter/pkg/config"
-	"get.porter.sh/porter/pkg/experimental"
 	"get.porter.sh/porter/pkg/manifest"
 	"get.porter.sh/porter/pkg/tracing"
 	buildx "github.com/docker/buildx/build"
@@ -144,7 +143,7 @@ func (b *Builder) BuildInvocationImage(ctx context.Context, manifest *manifest.M
 
 	out := ioutil.Discard
 	mode := progress.PrinterModeQuiet
-	if b.IsVerbose() || b.Config.IsFeatureEnabled(experimental.FlagStructuredLogs) {
+	if b.IsVerbose() {
 		mode = progress.PrinterModeAuto // Auto writes to stderr regardless of what you pass in
 
 		ctx, log = log.StartSpanWithName("buildkit", attribute.String("source", "porter.build.buildkit"))

pkg/config/config.go

@@ -101,8 +101,8 @@ func New() *Config {
 
 func (c *Config) NewLogConfiguration() portercontext.LogConfiguration {
 	return portercontext.LogConfiguration{
-		StructuredLogs:          c.IsFeatureEnabled(experimental.FlagStructuredLogs),
-		LogToFile:               c.Data.Logs.Enabled,
+		StructuredLogs:          c.Data.Logs.Structured,
+		LogToFile:               c.Data.Logs.LogToFile,
 		LogDirectory:            filepath.Join(c.porterHome, "logs"),
 		LogLevel:                c.Data.Logs.Level.Level(),
 		TelemetryEnabled:        c.Data.Telemetry.Enabled,

pkg/config/config_test.go

@@ -44,15 +44,15 @@ func TestConfig_GetHomeDirFromSymlink(t *testing.T) {
 func TestConfig_GetFeatureFlags(t *testing.T) {
 	t.Parallel()
 
-	t.Run("structured logs defaulted to disabled", func(t *testing.T) {
+	t.Run("feature defaulted to disabled", func(t *testing.T) {
 		c := Config{}
-		assert.False(t, c.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.False(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
-	t.Run("structured logs enabled", func(t *testing.T) {
+	t.Run("feature enabled", func(t *testing.T) {
 		c := Config{}
-		c.Data.ExperimentalFlags = []string{experimental.StructuredLogs}
-		assert.True(t, c.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		c.Data.ExperimentalFlags = []string{experimental.NoopFeature}
+		assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 }
 
@@ -61,33 +61,30 @@ func TestConfigExperimentalFlags(t *testing.T) {
 
 	t.Run("default off", func(t *testing.T) {
 		c := NewTestConfig(t)
-		assert.False(t, c.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.False(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
 	t.Run("user configuration", func(t *testing.T) {
-		os.Setenv("PORTER_EXPERIMENTAL", experimental.StructuredLogs)
+		os.Setenv("PORTER_EXPERIMENTAL", experimental.NoopFeature)
 		defer os.Unsetenv("PORTER_EXPERIMENTAL")
 
 		c := New()
 		require.NoError(t, c.Load(context.Background(), nil), "Load failed")
-		assert.True(t, c.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 
 	t.Run("programmatically enabled", func(t *testing.T) {
 		c := NewTestConfig(t)
 		c.Data.ExperimentalFlags = nil
-		c.SetExperimentalFlags(experimental.FlagStructuredLogs)
-		assert.True(t, c.IsFeatureEnabled(experimental.FlagStructuredLogs))
+		c.SetExperimentalFlags(experimental.FlagNoopFeature)
+		assert.True(t, c.IsFeatureEnabled(experimental.FlagNoopFeature))
 	})
 }
 
 func TestConfig_GetBuildDriver(t *testing.T) {
 	c := NewTestConfig(t)
 	c.Data.BuildDriver = "special"
 	require.Equal(t, BuildDriverBuildkit, c.GetBuildDriver(), "Default to docker when experimental is false, even when a build driver is set")
-
-	c.SetExperimentalFlags(experimental.FlagStructuredLogs)
-	require.Equal(t, BuildDriverBuildkit, c.GetBuildDriver(), "Use the specified driver when the build driver feature is enabled")
 }
 
 func TestConfig_ExportRemoteConfigAsEnvironmentVariables(t *testing.T) {
@@ -105,8 +102,9 @@ func TestConfig_ExportRemoteConfigAsEnvironmentVariables(t *testing.T) {
 	wantEnvVars := []string{
 		"PORTER_DEBUG=true",
 		"PORTER_DEBUG_PLUGINS=true",
-		"PORTER_LOGS_ENABLED=true",
 		"PORTER_LOGS_LEVEL=info",
+		"PORTER_LOGS_LOG_TO_FILE=true",
+		"PORTER_LOGS_STRUCTURED=true",
 		"PORTER_TELEMETRY_ENABLED=true",
 		"PORTER_TELEMETRY_REDIRECT_TO_FILE=true",
 	}

pkg/config/helpers.go

@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"get.porter.sh/porter/pkg"
-	"get.porter.sh/porter/pkg/experimental"
 	"get.porter.sh/porter/pkg/portercontext"
 	"get.porter.sh/porter/pkg/tracing"
 )
@@ -72,7 +71,6 @@ func (c *TestConfig) SetupIntegrationTest() (ctx context.Context, testDir string
 	// Check if telemetry should be enabled for the test
 	if telemetryEnabled, _ := strconv.ParseBool(os.Getenv("PORTER_TEST_TELEMETRY_ENABLED")); telemetryEnabled {
 		// Okay someone is listening, configure the tracer
-		c.SetExperimentalFlags(experimental.FlagStructuredLogs)
 		c.Data.Telemetry.Enabled = true
 		c.Data.Telemetry.Insecure = true
 		c.Data.Telemetry.Protocol = "grpc"

pkg/config/logs.go

@@ -8,8 +8,10 @@ import (
 
 // LogConfig are settings related to Porter's log files.
 type LogConfig struct {
-	Enabled bool     `mapstructure:"enabled"`
-	Level   LogLevel `mapstructure:"level"`
+	// Structured indicates if the logs sent to the console should include timestamp and log levels
+	Structured bool     `mapstructure:"structured"`
+	LogToFile  bool     `mapstructure:"log-to-file"`
+	Level      LogLevel `mapstructure:"level"`
 }
 
 // TelemetryConfig specifies how to connect to an open telemetry collector.

pkg/config/testdata/config.toml

@@ -6,14 +6,14 @@ default-storage = "dev"
 default-secrets-plugin = "azure.keyvault"
 default-secrets = "red-team"
 
-experimental = ["structured-logs"]
 
 # Keep this line because while there is only one build driver now, we may add more in the future
 build-driver = "buildkit"
 
 [logs]
-  enabled = true
+  log-to-file = true
   level = "info"
+  structured = true
 
 [telemetry]
   enabled = true

pkg/experimental/experimental.go

@@ -1,24 +1,25 @@
 package experimental
 
 const (
-	StructuredLogs = "structured-logs"
+	// NoopFeature is a placeholder feature flag that allows us to test our feature flag functions even when there are no active feature flags
+	NoopFeature = "no-op"
 )
 
 // FeatureFlags is an enum of possible feature flags
 type FeatureFlags int
 
 const (
-	// FlagStructuredLogs indicates if structured logs are enabled
-	FlagStructuredLogs FeatureFlags = iota + 1
+	// FlagNoopFeature is a placeholder feature flag that allows us to test our feature flag functions even when there are no active feature flags
+	FlagNoopFeature FeatureFlags = iota + 1
 )
 
 // ParseFlags converts a list of feature flag names into a bit map for faster lookups.
 func ParseFlags(flags []string) FeatureFlags {
 	var experimental FeatureFlags
 	for _, flag := range flags {
 		switch flag {
-		case StructuredLogs:
-			experimental = experimental | FlagStructuredLogs
+		case NoopFeature:
+			experimental = experimental | FlagNoopFeature
 		}
 	}
 	return experimental

tests/testdata/config/config-with-telemetry.yaml

@@ -2,7 +2,6 @@ namespace: dev
 debug: true
 debug-plugins: true
 default-storage: testdb
-experimental: [structured-logs]
 default-secrets-plugin: filesystem
 
 logs:

tests/testdata/config/config.yaml

@@ -2,7 +2,6 @@ namespace: dev
 debug: true
 debug-plugins: true
 default-storage: testdb
-experimental: [structured-logs]
 default-secrets-plugin: filesystem
 
 logs: