Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Commit

Permalink
Remove mandatory opencensus_task label from node and respect metric p…
Browse files Browse the repository at this point in the history 
…refix (#184)

* Remove opencensus_task label and respect metric prefix

* Avoid checking opencensus_task label on every req

* Fix review comments

* Pre-calculate default domain
  • Loading branch information
@songy23
songy23 authored Aug 29, 2019
1 parent 6c1c12f commit a8e01fd
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 75 deletions.
4 changes: 2 additions & 2 deletions metrics.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func (se *statsExporter) metricToMpbTs(ctx context.Context, metric *metricdata.M
resource := se.metricRscToMpbRsc(metric.Resource)

metricName := metric.Descriptor.Name
metricType, _ := se.metricTypeFromProto(metricName)
metricType := se.metricTypeFromProto(metricName)
metricLabelKeys := metric.Descriptor.LabelKeys
metricKind, _ := metricDescriptorTypeToMetricKind(metric)

Expand Down Expand Up @@ -238,7 +238,7 @@ func (se *statsExporter) metricToMpbMetricDescriptor(metric *metricdata.Metric)
return nil, errNilMetricOrMetricDescriptor
}

metricType, _ := se.metricTypeFromProto(metric.Descriptor.Name)
metricType := se.metricTypeFromProto(metric.Descriptor.Name)
displayName := se.displayName(metric.Descriptor.Name)
metricKind, valueType := metricDescriptorTypeToMetricKind(metric)

Expand Down
59 changes: 27 additions & 32 deletions metrics_proto.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ var percentileLabelKey = &metricspb.LabelKey{
Description: "the value at a given percentile of a distribution",
}
var globalResource = &resource.Resource{Type: "global"}
var domains = []string{"googleapis.com", "kubernetes.io", "istio.io"}

type metricProtoPayload struct {
node *commonpb.Node
Expand All @@ -75,17 +76,11 @@ func (se *statsExporter) ExportMetricsProto(ctx context.Context, node *commonpb.
return errNilMetricOrMetricDescriptor
}

additionalLabels := se.defaultLabels
if additionalLabels == nil {
// additionalLabels must be stateless because each node is different
additionalLabels = getDefaultLabelsFromNode(node)
}

for _, metric := range metrics {
if metric.GetMetricDescriptor().GetType() == metricspb.MetricDescriptor_SUMMARY {
se.addPayload(node, rsc, additionalLabels, se.convertSummaryMetrics(metric)...)
se.addPayload(node, rsc, se.defaultLabels, se.convertSummaryMetrics(metric)...)
} else {
se.addPayload(node, rsc, additionalLabels, metric)
se.addPayload(node, rsc, se.defaultLabels, metric)
}
}

Expand All @@ -102,12 +97,6 @@ func (se *statsExporter) ExportMetricsProtoSync(ctx context.Context, node *commo
// Caches the resources seen so far
seenResources := make(map[*resourcepb.Resource]*monitoredrespb.MonitoredResource)

additionalLabels := se.defaultLabels
if additionalLabels == nil {
// additionalLabels must be stateless because each node is different
additionalLabels = getDefaultLabelsFromNode(node)
}

ctx, cancel := se.o.newContextWithTimeout()
defer cancel()

Expand All @@ -123,14 +112,14 @@ func (se *statsExporter) ExportMetricsProtoSync(ctx context.Context, node *commo
if metric.GetMetricDescriptor().GetType() == metricspb.MetricDescriptor_SUMMARY {
summaryMtcs := se.convertSummaryMetrics(metric)
for _, summaryMtc := range summaryMtcs {
if tss, err := se.protoMetricToTimeSeries(ctx, mappedRsc, summaryMtc, additionalLabels); err == nil {
if tss, err := se.protoMetricToTimeSeries(ctx, mappedRsc, summaryMtc, se.defaultLabels); err == nil {
allTss = append(tss, tss...)
} else {
allErrs = append(allErrs, err)
}
}
} else {
if tss, err := se.protoMetricToTimeSeries(ctx, mappedRsc, metric, additionalLabels); err == nil {
if tss, err := se.protoMetricToTimeSeries(ctx, mappedRsc, metric, se.defaultLabels); err == nil {
allTss = append(allTss, tss...)
} else {
allErrs = append(allErrs, err)
Expand Down Expand Up @@ -450,7 +439,7 @@ func (se *statsExporter) protoMetricToTimeSeries(ctx context.Context, mappedRsc
}

metricName := metric.GetMetricDescriptor().GetName()
metricType, _ := se.metricTypeFromProto(metricName)
metricType := se.metricTypeFromProto(metricName)
metricLabelKeys := metric.GetMetricDescriptor().GetLabelKeys()
metricKind, valueType := protoMetricDescriptorTypeToMetricKind(metric)
labelKeys := make([]string, len(metricLabelKeys))
Expand Down Expand Up @@ -594,7 +583,7 @@ func (se *statsExporter) protoToMonitoringMetricDescriptor(metric *metricspb.Met
metricName := md.GetName()
unit := md.GetUnit()
description := md.GetDescription()
metricType, _ := se.metricTypeFromProto(metricName)
metricType := se.metricTypeFromProto(metricName)
displayName := se.displayName(metricName)
metricKind, valueType := protoMetricDescriptorTypeToMetricKind(metric)

Expand Down Expand Up @@ -635,10 +624,26 @@ func labelDescriptorsFromProto(defaults map[string]labelValue, protoLabelKeys []
return labelDescriptors
}

func (se *statsExporter) metricTypeFromProto(name string) (string, bool) {
// TODO: (@odeke-em) support non-"custom.googleapis.com" metrics names.
name = path.Join("custom.googleapis.com", "opencensus", name)
return name, true
func (se *statsExporter) metricTypeFromProto(name string) string {
prefix := se.o.MetricPrefix
if prefix != "" {
name = prefix + name
}
if !hasDomain(name) {
// Still needed because the name may or may not have a "/" at the beginning.
name = path.Join(defaultDomain, name)
}
return name
}

// hasDomain checks if the metric name already has a domain in it.
func hasDomain(name string) bool {
for _, domain := range domains {
if strings.Contains(name, domain) {
return true
}
}
return false
}

func fromProtoPoint(startTime *timestamp.Timestamp, pt *metricspb.Point) (*monitoringpb.Point, error) {
Expand Down Expand Up @@ -776,16 +781,6 @@ func protoMetricDescriptorTypeToMetricKind(m *metricspb.Metric) (googlemetricpb.
}
}

func getDefaultLabelsFromNode(node *commonpb.Node) map[string]labelValue {
taskValue := fmt.Sprintf("%s-%d@%s", strings.ToLower(node.LibraryInfo.GetLanguage().String()), node.Identifier.Pid, node.Identifier.HostName)
return map[string]labelValue{
sanitize(opencensusTaskKey): {
val: taskValue,
desc: opencensusTaskDescription,
},
}
}

// combineErrors converts a list of errors into one error.
func combineErrors(errs []error) error {
numErrors := len(errs)
Expand Down
82 changes: 41 additions & 41 deletions metrics_proto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package stackdriver

import (
"context"
"reflect"
"strings"
"testing"

Expand All @@ -29,7 +28,6 @@ import (
monitoredrespb "google.golang.org/genproto/googleapis/api/monitoredres"
monitoringpb "google.golang.org/genproto/googleapis/monitoring/v3"

commonpb "github.com/census-instrumentation/opencensus-proto/gen-go/agent/common/v1"
metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
"github.com/golang/protobuf/ptypes/wrappers"
"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -632,45 +630,6 @@ func TestCombineTimeSeriesAndDeduplication(t *testing.T) {
}
}

func TestNodeToDefaultLabels(t *testing.T) {
tests := []struct {
in *commonpb.Node
want map[string]labelValue
}{
{
in: &commonpb.Node{
Identifier: &commonpb.ProcessIdentifier{HostName: "host1", Pid: 8081},
LibraryInfo: &commonpb.LibraryInfo{Language: commonpb.LibraryInfo_JAVA},
},
want: map[string]labelValue{
"opencensus_task": {
val: "java-8081@host1",
desc: "Opencensus task identifier",
},
},
},
{
in: &commonpb.Node{
Identifier: &commonpb.ProcessIdentifier{HostName: "host2", Pid: 9090},
LibraryInfo: &commonpb.LibraryInfo{Language: commonpb.LibraryInfo_PYTHON},
},
want: map[string]labelValue{
"opencensus_task": {
val: "python-9090@host2",
desc: "Opencensus task identifier",
},
},
},
}

for i, tt := range tests {
got := getDefaultLabelsFromNode(tt.in)
if !reflect.DeepEqual(got, tt.want) {
t.Fatalf("Test %d failed. Default labels mismatch. Want %v\nGot %v\n", i, tt.want, got)
}
}
}

func TestConvertSummaryMetrics(t *testing.T) {
startTimestamp := &timestamp.Timestamp{
Seconds: 1543160298,
Expand Down Expand Up @@ -790,6 +749,47 @@ func TestConvertSummaryMetrics(t *testing.T) {
}
}

func TestMetricPrefix(t *testing.T) {
tests := []struct {
name string
in string
want string
statsExporter *statsExporter
}{
{
name: "No prefix and metric name has a kubernetes domain",
in: "kubernetes.io/container/memory/limit_bytes",
statsExporter: &statsExporter{
o: Options{ProjectID: "foo"},
},
want: "kubernetes.io/container/memory/limit_bytes",
},
{
name: "Has a prefix but prefix doesn't have a domain",
in: "my_metric",
statsExporter: &statsExporter{
o: Options{ProjectID: "foo", MetricPrefix: "prefix/"},
},
want: "custom.googleapis.com/opencensus/prefix/my_metric",
},
{
name: "Has a prefix and prefix has a domain",
in: "my_metric",
statsExporter: &statsExporter{
o: Options{ProjectID: "foo", MetricPrefix: "appengine.googleapis.com/"},
},
want: "appengine.googleapis.com/my_metric",
},
}

for _, tt := range tests {
got := tt.statsExporter.metricTypeFromProto(tt.in)
if !cmp.Equal(got, tt.want) {
t.Fatalf("mismatch metric names for test %v:\n got=%v\n want=%v\n", tt.name, got, tt.want)
}
}
}

func makeInt64Ts(val int64, label string, start, end *timestamp.Timestamp) *metricspb.TimeSeries {
ts := &metricspb.TimeSeries{
StartTimestamp: start,
Expand Down
6 changes: 6 additions & 0 deletions stackdriver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import (
"log"
"os"
"path"
"strings"
"time"

metadataapi "cloud.google.com/go/compute/metadata"
Expand Down Expand Up @@ -263,6 +264,8 @@ type Options struct {

const defaultTimeout = 5 * time.Second

var defaultDomain = path.Join("custom.googleapis.com", "opencensus")

// Exporter is a stats and trace exporter that uploads data to Stackdriver.
//
// You can create a single Exporter and register it as both a trace exporter
Expand Down Expand Up @@ -336,6 +339,9 @@ func NewExporter(o Options) (*Exporter, error) {

o.Resource = o.MapResource(res)
}
if o.MetricPrefix != "" && !strings.HasSuffix(o.MetricPrefix, "/") {
o.MetricPrefix = o.MetricPrefix + "/"
}

se, err := newStatsExporter(o)
if err != nil {
Expand Down

0 comments on commit a8e01fd

Please sign in to comment.