Basic implementation of OpenCensus #1414

DazWilkin · 2019-01-22T23:00:45Z

Implementation of OpenCensus monitoring (alternative to Prometheus-specific monitoring and a complement to the existing OpenCensus trace).

The code needs work.

I'm interested in feedback on the implementation and its usefulness.

Checklist

I have updated the CHANGELOG.
- Adjust the draft version number according to semantic versioning rules.
I have updated documentation accordingly.

AlCutter

Thanks Daz!
I think this is a great idea, could potentially kill off a bunch of complexity eventually by pulling everything under opencensus.

I popped a few light comments in-line, though I know you said the code's not really ready yet.

AlCutter · 2019-01-23T12:09:11Z

monitoring/opencensus/metrics.go

+		log.Fatal(err)
+	}
+	// Important to invoke Flush before exiting
+	defer sd.Flush()


You probably want to have this func not be init but an exported Initialise func which itself returns a func() to be called at shutdown time, e.g.

func Initialize() (func(), error) { ... return func() { if using SD { sd.Flush() } if using DD { dd.Stop() } }, nil }

then in the various server mains:

... var mf monitoring.MetricFactory if *useOpencensus { flush, err := opencensus.Initialise() // if err etc. defer flush() mf = &opencensus.MetricFactory{..} } ...

Otherwise, as it stands, I think it'll just flush anything that's been logged during the course of the init method when the init returns.

Addressed thanks to your guidance: 2706a6e

This requires changes to each server main which would break the Prometheus implementation. So, leaving unchanged for now. Must also understand how best to reflect the configuration through Trillian's configuration.

//TODO(dazwilkin) Datadog, Prometheus & Stackdriver should be configurable if true { flush, err := opencensus.Initialize() if err != nil { glog.Exitf("Failed to initialize OpenCensus Exporter(s): %v", err) } defer flush() }

monitoring/opencensus/metrics.go

AlCutter · 2019-01-23T12:28:03Z

monitoring/opencensus/metrics.go

+		MetricPrefix: namespace,
+	})
+	if err != nil {
+		log.Fatal(err)


There's a mix of log and glog through here, is that because you don't want to rely on glog being usable in init?

AlCutter · 2019-01-23T12:28:51Z

monitoring/opencensus/metrics.go

+	}
+}
+
+// createKeyMethods creates OpenCensus Tag Keys for each label name


createTagKeys :)

Fixed: c44fd34

AlCutter · 2019-01-23T12:29:35Z

monitoring/opencensus/metrics.go

+// -- contains non-printable ASCII
+// -- or len is 0 or >256
+// Printable ASCII 32-126 inclusive
+func checkLabelNames(labelNames []string) {


nit: labelNames -> names or ln to reduce stutter

Fixed: c6436c0

AlCutter · 2019-01-23T12:31:57Z

monitoring/opencensus/metrics.go

+	return tagKeys
+}
+
+// createMeasureAndView creates the OpenCensus Measure used to record stats and a View for reporting them


(nit missing full stops in comment)

Fixed: d5ab0a5

AlCutter · 2019-01-23T12:34:34Z

monitoring/opencensus/metrics.go

+
+// forAllLabelsAValue trivially ensures the numbers of labels matches the number of values
+func forAllLabelsAValue(labels, values []string) error {
+	if len(labels) != len(values) {


can reduce clutter and re-calculation a bit like this:

if ll, lv := len(labels), len(values); ll != lv { return fmt.Errorf(..., ll, lv) }

Fixed: e487093

AlCutter · 2019-01-23T12:37:03Z

monitoring/opencensus/metrics.go

+	return r
+}
+
+// NewHistogram creates a new Histogram object backed by Prometheus.


/Prometheus/OpenCensus/?

Fixed: acc264c

AlCutter · 2019-01-23T12:42:32Z

monitoring/opencensus/metrics.go

+}
+
+// Inc adds 1 to the gauge
+func (g *Gauge) Inc(labelVals ...string) {


It's possible these are unused, or calling code could be refactored to call Set (github search makes it hard to figure that out, vi and go build would do a better job :))

I don't understand this comment but I think perhaps you allude to this issue:

One challenge implementing the Trillian metric interface with OpenCensus is that the interface assumes the possibility of tracking state of the measure. Unlike with Prometheus (where this is possible), OpenCensus is a proxy (for one or more monitoring services) and the proxy simply dispatches the current value to the configured backends without recording the current measure's value.

As a result, it's possible to increment Counters, and set the value of Gauges. But it's not possible to get the value of either, and thus it's not possible to increment, decrement of add values to e.g. Gauges because this requires retaining knowledge of the current value.

If the interface is changed to reflect this, then the Prometheus tests that assume this, will break. As it is, the OpenCensus tests break because it's not possible to get the measure values.

DazWilkin · 2019-01-23T16:08:23Z

@AlCutter --

Yes, was seeking feedback and guidance, thank you.

I'm headed out on vacation but will incorporate your feedback when I'm back.

DazWilkin · 2019-02-06T22:13:20Z

@AlCutter --

This isn't forgotten.

I'm now back Stateside and have been working with @gdbelvin on Key Transparency.

I'll make the changes you propose and resubmit before the end of next week.

DazWilkin · 2019-02-14T18:12:25Z

Apologies... a further week's delay.

We lost power this week during Seattle's Snowmaggedon and it's set me back

DazWilkin · 2019-02-28T20:43:41Z

OpenCensus now provides an Agent.

Among other features (including accepting incoming Prometheus metric data), this proxies OpenCensus metric data to OpenCensus Exporters.

This means that, instead of -- as currently -- having to supported multiple Exporters (Stackdriver, Prometheus, Datadog) in the Trillian code, the code may be simplified to write to a generic OpenCensus exporter that itself writes to the Agent. At run|deploy time, the operator can decide which monitoring|trace services to configure.

I'll simplify the code to use the generic Exporter and re-commit.

golangcibot · 2019-04-03T16:41:55Z

monitoring/opencensus/metrics.go

+)
+
+const (
+	namespace = "trillian"


namespace is unused (from deadcode)

Fixed: 30c7a29

AlCutter · 2019-04-03T17:43:12Z

monitoring/opencensus/metrics.go

-		if strings.IndexFunc(labelName, nonPrintableASCII) != -1 {
-			glog.Fatalf("OpenCensus label names must be printable ASCII; '%s' is not", labelName)
+		if strings.IndexFunc(name, nonPrintableASCII) != -1 {
+			glog.Fatalf("OpenCensus label names must be printable ASCII; '%s' is not", name)


nitty nit: can use %q rather than '%s' for the same output.

Thanks!

I did not know that... my printf foo is weak.

Fixed: 8986d5a

AlCutter · 2019-04-04T14:33:44Z

Looks like it's missing an interface method:

monitoring/opencensus/metrics_test.go:12:39: cannot use MetricFactory literal (type MetricFactory) as type "github.com/google/trillian/monitoring".MetricFactory in argument to testonly.TestCounter:
	MetricFactory does not implement "github.com/google/trillian/monitoring".MetricFactory (missing NewHistogramWithBuckets method)

DazWilkin · 2019-04-04T20:27:49Z

There's an issue in the OpenCensus Stackdriver Exporter. If buckets are defined to include a 0-valued bucket i.e. [0.0, ....], the Exporter fails and no metric data is sent to Stackdriver: census-ecosystem/opencensus-go-exporter-stackdriver#98

This Trillian OpenCensus code now ships metrics to an OpenCensus Agent. The Trillian code is unable to determine whether the Agent is configured to export to Stackdriver.

To avoid this error (and assuming -- !! -- that this doesn't impact the other Exporters), the code currently logs the fact and deletes the 0-valued bucket from the list of buckets.

NB The Interface doesn't support and the existing Prometheus code doesn't check for a histogram created with zero buckets. Neither does this OpenCensus code (though it does log the fact). This appears to be an error case.

AlCutter · 2019-04-17T11:18:09Z

Probably need a rebase on this - Travis is failing due to an obsolete workaround still being attempted.

RJPercival · 2019-10-22T09:57:07Z

Hi @DazWilkin, is this stalled on the linked exporter issue or does it just need some work to rebase it and get it merged?

DazWilkin · 2019-10-23T17:24:03Z

I'm just back from vacation.

I am unclear what failed.

I'll try to recreate this, review and update this thread by the end of this week, OK?

DazWilkin · 2019-10-25T20:45:39Z

@RJPercival tested and confirmed that it continues to work.

Trillian will need an additional go.mod entry:

contrib.go.opencensus.io/exporter/ocagent v0.6.0

I am unclear why this was failing CI but I've removed the tests.

OpenCensus ships metrics asynchronously to exporters (in this case, the OpenCensus Agent). Thus it has no (easy) way to retrieve measurement values and thus it's unable to succeed with the monitoring tests as defined.

I've revised the README.md to explain configuration, but in summary:

Add a flag to the server main.go to define the OpenCensus Agent endpoint
Instantiate it
Initialize it (!) providing the Agent's endpoint and defer shutdown.

var (
	ocatEndpoint    = flag.String("ocat_endpoint", "localhost:55678", "Endpoint for OpenCensus Agent receiver (host:port)")
)

...

mf := opencensus.MetricFactory{}

flush, err := opencensus.Initialize(*ocatEndpoint)
if err != nil {
	glog.Exitf("Failed to initialize OpenCensus Agent: %v", err)
}
defer flush()

NB Because it no longer defaults to (only use) Prometheus, the server's HTTP endpoint no longer includes the /metrics endpoint. If the Agent is configured to export to Prometheus, these metrics will be merged into the Agent's :55678/metrics

AlCutter · 2021-01-21T15:11:26Z

Hey Daz, we're just cleaning out old PR/issues - there's clearly a bunch of work which went into this, is it worth trying to bump this up to date and land it or is it less useful now?

DazWilkin · 2021-01-21T17:04:26Z

As you know, OpenCensus is superseded by OpenTelemetry which, IIRC, implements a metric format that's very close to Prometheus' exposition format (maybe even the same?).

I think this PR should be dropped and Trillian may either export directly as an OpenTelemetry exporter or worst-case use the OpenTelemetry collector.

AlCutter · 2021-01-21T19:11:48Z

Cool, thanks Daz!

Basic implementation of OpenCensus

795a2f4

googlebot added cla: yes labels Jan 22, 2019

AlCutter reviewed Jan 23, 2019

View reviewed changes

RJPercival assigned DazWilkin Feb 4, 2019

Daz Wilkin added 6 commits February 22, 2019 09:56

Replaced incorrect init with Initialize per AlCutter@

2706a6e

Corrected comment

c44fd34

Renamed labelNames to names to reduce stutter per AlCutter@

c6436c0

Tidied up the comments (with punctuation) per AlCutter@

d5ab0a5

More efficient and no duplication

e487093

s/Prometheus/OpenCensus

acc264c

Replaced Exporters with OpenCensus Agent Exporter

b485286

golangcibot reviewed Apr 3, 2019

View reviewed changes

Removed "namespace"; Added Agent finalizer

30c7a29

AlCutter reviewed Apr 3, 2019

View reviewed changes

%q=='%s' IDNKT

8986d5a

Daz Wilkin added 3 commits April 4, 2019 10:41

Merge branch 'master' into monitoring-opencensus

8eb3cac

Interface changed; Added NewHistogramWithBuckets

39f3d03

Added APIs Explorer samples to README

0dd20cf

DazWilkin added 2 commits October 23, 2019 10:49

Bring monitoring-opencensus up-to-date w/ Trillian master

ae0d675

Minor tweaks. Updated README. Voided tests (!)

052cd37

AlCutter assigned RJPercival Nov 6, 2019

gdbelvin assigned AlCutter Jun 30, 2020

AlCutter unassigned RJPercival Jan 21, 2021

AlCutter closed this Jan 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic implementation of OpenCensus #1414

Basic implementation of OpenCensus #1414

DazWilkin commented Jan 22, 2019

AlCutter left a comment

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

AlCutter Jan 23, 2019

DazWilkin Feb 22, 2019

DazWilkin commented Jan 23, 2019

DazWilkin commented Feb 6, 2019

DazWilkin commented Feb 14, 2019

DazWilkin commented Feb 28, 2019

golangcibot Apr 3, 2019

DazWilkin Apr 3, 2019

AlCutter Apr 3, 2019

DazWilkin Apr 3, 2019

AlCutter commented Apr 4, 2019

DazWilkin commented Apr 4, 2019 •

edited

Loading

AlCutter commented Apr 17, 2019

RJPercival commented Oct 22, 2019

DazWilkin commented Oct 23, 2019

DazWilkin commented Oct 25, 2019

AlCutter commented Jan 21, 2021

DazWilkin commented Jan 21, 2021

AlCutter commented Jan 21, 2021

Basic implementation of OpenCensus #1414

Basic implementation of OpenCensus #1414

Conversation

DazWilkin commented Jan 22, 2019

Checklist

AlCutter 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

Choose a reason for hiding this comment

DazWilkin commented Jan 23, 2019

DazWilkin commented Feb 6, 2019

DazWilkin commented Feb 14, 2019

DazWilkin commented Feb 28, 2019

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

AlCutter commented Apr 4, 2019

DazWilkin commented Apr 4, 2019 • edited Loading

AlCutter commented Apr 17, 2019

RJPercival commented Oct 22, 2019

DazWilkin commented Oct 23, 2019

DazWilkin commented Oct 25, 2019

AlCutter commented Jan 21, 2021

DazWilkin commented Jan 21, 2021

AlCutter commented Jan 21, 2021

DazWilkin commented Apr 4, 2019 •

edited

Loading