server: Remove deprecated diagnostic feature

This commit removes the deprecated diagnostic feature from the
server. The feature has been deprecated since November 2018 and it was
essentially unused at the time so it should be safe to
remove. Removing the diagnostic support from the server saves having
to perform an extra policy evaluation in the server.

Once the buffer is removed from the runtime.Params struct the related
issue can be closed (there is still one known user of that so it has
been left intact for backwards compatibility.)

Ref open-policy-agent#1052

Signed-off-by: Torin Sandall <torinsandall@gmail.com>

cmd/run.go

@@ -21,9 +21,8 @@ import (
 )
 
 const (
-	defaultAddr                        = ":8181"        // default listening address for server
-	defaultHistoryFile                 = ".opa_history" // default filename for shell history
-	defaultServerDiagnosticsBufferSize = 10             // default number of items to keep in diagnostics buffer for server
+	defaultAddr        = ":8181"        // default listening address for server
+	defaultHistoryFile = ".opa_history" // default filename for shell history
 )
 
 func init() {
@@ -47,8 +46,6 @@ func init() {
 		"off":   server.AuthorizationOff,
 	}
 
-	var serverDiagnosticsBufferSize int
-
 	logLevel := util.NewEnumFlag("info", []string{"debug", "info", "error"})
 	logFormat := util.NewEnumFlag("json", []string{"text", "json", "json-pretty"})
 
@@ -114,7 +111,6 @@ the data document with the following syntax:
 				Level:  logLevel.String(),
 				Format: logFormat.String(),
 			}
-			params.DiagnosticsBuffer = server.NewBoundedBuffer(serverDiagnosticsBufferSize)
 			params.Paths = args
 			params.Filter = loaderFilter{
 				Ignore: ignore,
@@ -146,8 +142,6 @@ the data document with the following syntax:
 	runCommand.Flags().BoolVarP(&params.Watch, "watch", "w", false, "watch command line files for changes")
 	setMaxErrors(runCommand.Flags(), &params.ErrorLimit)
 	runCommand.Flags().BoolVarP(&params.PprofEnabled, "pprof", "", false, "enables pprof endpoints")
-	runCommand.Flags().IntVarP(&serverDiagnosticsBufferSize, "server-diagnostics-buffer-size", "", defaultServerDiagnosticsBufferSize, "set the size of the server's diagnostics buffer")
-	runCommand.Flags().MarkDeprecated("server-diagnostics-buffer-size", "use decision logging instead")
 	runCommand.Flags().StringVarP(&tlsCertFile, "tls-cert-file", "", "", "set path of TLS certificate file")
 	runCommand.Flags().StringVarP(&tlsPrivateKeyFile, "tls-private-key-file", "", "", "set path of TLS private key file")
 	runCommand.Flags().StringVarP(&tlsCACertFile, "tls-ca-cert-file", "", "", "set path of TLS CA cert file")

docs/content/monitoring.md

@@ -26,9 +26,4 @@ scrape_configs:
 ## Health Checks
 
 OPA exposes a `/health` API endpoint that can be used to perform health checks.
-See [Health API](/docs/{{< current_version >}}/rest-api#health-api) for details.
-
-## Diagnostics (Deprecated)
-
-The diagnostics feature is deprecated. If you need to monitor OPA decisions, see
-the [Decision Log](../decision-logs) API.
+See [Health API](/docs/{{< current_version >}}/rest-api#health-api) for details.

runtime/runtime.go

@@ -111,6 +111,7 @@ type Params struct {
 	DecisionIDFactory func() string
 
 	// DiagnosticsBuffer is used by the server to record policy decisions.
+	// DEPRECATED. Use decision logging instead.
 	DiagnosticsBuffer server.Buffer
 
 	// Logging configures the logging behaviour.
@@ -276,7 +277,6 @@ func (rt *Runtime) Serve(ctx context.Context) error {
 		WithCertPool(rt.Params.CertPool).
 		WithAuthentication(rt.Params.Authentication).
 		WithAuthorization(rt.Params.Authorization).
-		WithDiagnosticsBuffer(rt.Params.DiagnosticsBuffer).
 		WithDecisionIDFactory(rt.decisionIDFactory).
 		WithDecisionLoggerWithErr(rt.decisionLogger).
 		WithRuntime(rt.info).
@@ -368,10 +368,16 @@ func (rt *Runtime) decisionIDFactory() string {
 }
 
 func (rt *Runtime) decisionLogger(ctx context.Context, event *server.Info) error {
+
+	if rt.Params.DiagnosticsBuffer != nil {
+		rt.Params.DiagnosticsBuffer.Push(event)
+	}
+
 	plugin := logs.Lookup(rt.Manager)
 	if plugin == nil {
 		return nil
 	}
+
 	return plugin.Log(ctx, event)
 }
 
@@ -493,18 +499,9 @@ func compileAndStoreInputs(ctx context.Context, store storage.Store, txn storage
 		}
 	}
 
-	warnDiagnosticPolicyDeprecated(c)
-
 	return nil
 }
 
-func warnDiagnosticPolicyDeprecated(c *ast.Compiler) {
-	rules := c.GetRules(ast.MustParseRef("data.system.diagnostics"))
-	if len(rules) > 0 {
-		logrus.Warn("The diagnostics feature has been deprecated and will be removed. Use the Decision Logging feature. See https://www.openpolicyagent.org/docs/decision_logs.html for information.")
-	}
-}
-
 func getWatcher(rootPaths []string) (*fsnotify.Watcher, error) {
 
 	watchPaths, err := getWatchPaths(rootPaths)

server/buffer.go

@@ -5,17 +5,15 @@
 package server
 
 import (
-	"sync"
 	"time"
 
 	"github.com/open-policy-agent/opa/metrics"
 	"github.com/open-policy-agent/opa/storage"
 	"github.com/open-policy-agent/opa/topdown"
 )
 
-// Buffer defines an interface that the server can call to push diagnostic
-// information about policy decisions. Buffers must be able to handle
-// concurrent calls.
+// Buffer defines an interface for recording decisions.
+// DEPRECATED. Use Decision Logging instead.
 type Buffer interface {
 	// Push adds the given Info into the buffer.
 	Push(*Info)
@@ -25,52 +23,6 @@ type Buffer interface {
 	Iter(fn func(*Info))
 }
 
-// buffer stores diagnostic information.
-type buffer struct {
-	ring  []*Info
-	size  int
-	start int // The index of the next item to pop.
-	end   int // The index where the next item will be inserted.
-	sync.Mutex
-}
-
-// NewBoundedBuffer creates a new Buffer with maximum size n. NewBoundedBuffer
-// will panic if n is not positive.
-func NewBoundedBuffer(n int) Buffer {
-	if n <= 0 {
-		panic("size must be greater than 0")
-	}
-
-	return &buffer{ring: make([]*Info, n, n)}
-}
-
-func (b *buffer) Push(i *Info) {
-	b.Lock()
-	defer b.Unlock()
-
-	b.ring[b.end] = i
-
-	b.end = (b.end + 1) % len(b.ring)
-
-	switch b.size {
-	case len(b.ring): // We erased something, move the start up.
-		b.start = (b.start + 1) % len(b.ring)
-	default:
-		b.size++
-	}
-}
-
-func (b *buffer) Iter(fn func(*Info)) {
-	b.Lock()
-	defer b.Unlock()
-
-	i := b.start
-	for j := 0; j < b.size; j++ {
-		fn(b.ring[i])
-		i = (i + 1) % len(b.ring)
-	}
-}
-
 // Info contains information describing a policy decision.
 type Info struct {
 	Txn        storage.Transaction