exporter/signalfx: Correctly convert dimensions on metadata updates (#…

…2552)

exporter/signalfxexporter/dimensions/dimclient.go

@@ -72,7 +72,7 @@ type DimensionClient struct {
  TotalSuccessfulUpdates int64
  logUpdates bool
  logger *zap.Logger
- metricTranslator *translation.MetricTranslator
+ metricsConverter translation.MetricsConverter
 }
 
 type queuedDimension struct {
@@ -87,12 +87,11 @@ type DimensionClientOptions struct {
  Logger *zap.Logger
  SendDelay int
  PropertiesMaxBuffered int
- MetricTranslator *translation.MetricTranslator
+ MetricsConverter translation.MetricsConverter
 }
 
 // NewDimensionClient returns a new client
 func NewDimensionClient(ctx context.Context, options DimensionClientOptions) *DimensionClient {
-
  client := &http.Client{
  Timeout: 10 * time.Second,
  Transport: &http.Transport{
@@ -122,7 +121,7 @@ func NewDimensionClient(ctx context.Context, options DimensionClientOptions) *Di
  now: time.Now,
  logger: options.Logger,
  logUpdates: options.LogUpdates,
- metricTranslator: options.MetricTranslator,
+ metricsConverter: options.MetricsConverter,
  }
 }
 

exporter/signalfxexporter/dimensions/metadata.go

@@ -32,25 +32,17 @@ type MetadataUpdateClient interface {
 
 func getDimensionUpdateFromMetadata(
  metadata metadata.MetadataUpdate,
- metricTranslator *translation.MetricTranslator,
-) *DimensionUpdate {
+ metricsConverter translation.MetricsConverter) *DimensionUpdate {
  properties, tags := getPropertiesAndTags(metadata)
 
  return &DimensionUpdate{
- Name: translateDimension(metricTranslator, metadata.ResourceIDKey),
+ Name: metricsConverter.ConvertDimension(metadata.ResourceIDKey),
  Value: string(metadata.ResourceID),
  Properties: properties,
  Tags: tags,
  }
 }
 
-func translateDimension(metricTranslator *translation.MetricTranslator, dim string) string {
- if metricTranslator != nil {
- return metricTranslator.TranslateDimension(dim)
- }
- return dim
-}
-
 const (
  oTelK8sServicePrefix = "k8s.service."
  sfxK8sServicePrefix = "kubernetes_service_"
@@ -116,7 +108,7 @@ func getPropertiesAndTags(kmu metadata.MetadataUpdate) (map[string]*string, map[
 func (dc *DimensionClient) PushMetadata(metadata []*metadata.MetadataUpdate) error {
  var errs []error
  for _, m := range metadata {
- dimensionUpdate := getDimensionUpdateFromMetadata(*m, dc.metricTranslator)
+ dimensionUpdate := getDimensionUpdateFromMetadata(*m, dc.metricsConverter)
 
  if dimensionUpdate.Name == "" || dimensionUpdate.Value == "" {
  atomic.AddInt64(&dc.TotalInvalidDimensions, int64(1))

exporter/signalfxexporter/dimensions/metadata_test.go

@@ -18,6 +18,9 @@ import (
  "reflect"
  "testing"
 
+ "github.com/stretchr/testify/require"
+ "go.uber.org/zap"
+
  "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/signalfxexporter/translation"
  metadata "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/experimentalmetricmetadata"
 )
@@ -205,7 +208,16 @@ func TestGetDimensionUpdateFromMetadata(t *testing.T) {
  }
  for _, tt := range tests {
  t.Run(tt.name, func(t *testing.T) {
- got := getDimensionUpdateFromMetadata(tt.args.metadata, tt.args.metricTranslator)
+
+ converter, err := translation.NewMetricsConverter(
+ zap.NewNop(),
+ tt.args.metricTranslator,
+ nil,
+ nil,
+ "-_.",
+ )
+ require.NoError(t, err)
+ got := getDimensionUpdateFromMetadata(tt.args.metadata, *converter)
  if !reflect.DeepEqual(*got, *tt.want) {
  t.Errorf("getDimensionUpdateFromMetadata() = %v, want %v", *got, *tt.want)
  }

exporter/signalfxexporter/exporter.go

@@ -125,7 +125,7 @@ func newSignalFxExporter(
  // buffer a fixed number of updates. Might also be a good candidate
  // to make configurable.
  PropertiesMaxBuffered: 10000,
- MetricTranslator: options.metricTranslator,
+ MetricsConverter: *converter,
  })
  dimClient.Start()
 

exporter/signalfxexporter/exporter_test.go

@@ -25,6 +25,7 @@ import (
  "net/url"
  "sort"
  "strconv"
+ "strings"
  "sync"
  "testing"
  "time"
@@ -755,20 +756,31 @@ func generateLargeEventBatch() pdata.Logs {
 }
 
 func TestConsumeMetadata(t *testing.T) {
+ cfg := createDefaultConfig().(*Config)
+ converter, err := translation.NewMetricsConverter(
+ zap.NewNop(),
+ nil,
+ cfg.ExcludeMetrics,
+ cfg.IncludeMetrics,
+ cfg.NonAlphanumericDimensionChars,
+ )
+ require.NoError(t, err)
  type args struct {
  metadata []*metadata.MetadataUpdate
  }
  type fields struct {
  payLoad map[string]interface{}
  }
  tests := []struct {
- name string
- fields fields
- args args
+ name string
+ fields fields
+ args args
+ expectedDimensionKey string
+ expectedDimensionValue string
  }{
  {
- "Test property updates",
- fields{
+ name: "Test property updates",
+ fields: fields{
  map[string]interface{}{
  "customProperties": map[string]interface{}{
  "prop.erty1": "val1",
@@ -780,7 +792,7 @@ func TestConsumeMetadata(t *testing.T) {
  "tagsToRemove": nil,
  },
  },
- args{
+ args: args{
  []*metadata.MetadataUpdate{
  {
  ResourceIDKey: "key",
@@ -800,10 +812,12 @@ func TestConsumeMetadata(t *testing.T) {
  },
  },
  },
+ expectedDimensionKey: "key",
+ expectedDimensionValue: "id",
  },
  {
- "Test tag updates",
- fields{
+ name: "Test tag updates",
+ fields: fields{
  map[string]interface{}{
  "customProperties": map[string]interface{}{},
  "tags": []interface{}{
@@ -814,7 +828,7 @@ func TestConsumeMetadata(t *testing.T) {
  },
  },
  },
- args{
+ args: args{
  []*metadata.MetadataUpdate{
  {
  ResourceIDKey: "key",
@@ -831,10 +845,12 @@ func TestConsumeMetadata(t *testing.T) {
  },
  },
  },
+ expectedDimensionKey: "key",
+ expectedDimensionValue: "id",
  },
  {
- "Test quick successive updates",
- fields{
+ name: "Test quick successive updates",
+ fields: fields{
  map[string]interface{}{
  "customProperties": map[string]interface{}{
  "property1": nil,
@@ -849,7 +865,7 @@ func TestConsumeMetadata(t *testing.T) {
  },
  },
  },
- args{
+ args: args{
  []*metadata.MetadataUpdate{
  {
  ResourceIDKey: "key",
@@ -898,6 +914,45 @@ func TestConsumeMetadata(t *testing.T) {
  },
  },
  },
+ expectedDimensionKey: "key",
+ expectedDimensionValue: "id",
+ },
+ {
+ name: "Test updates on dimensions with nonalphanumeric characters (other than the default allow list)",
+ fields: fields{
+ map[string]interface{}{
+ "customProperties": map[string]interface{}{
+ "prop.erty1": "val1",
+ "property2": nil,
+ "prop.erty3": "val33",
+ "property4": nil,
+ },
+ "tags": nil,
+ "tagsToRemove": nil,
+ },
+ },
+ args: args{
+ []*metadata.MetadataUpdate{
+ {
+ ResourceIDKey: "k!e=y",
+ ResourceID: "id",
+ MetadataDelta: metadata.MetadataDelta{
+ MetadataToAdd: map[string]string{
+ "prop.erty1": "val1",
+ },
+ MetadataToRemove: map[string]string{
+ "property2": "val2",
+ },
+ MetadataToUpdate: map[string]string{
+ "prop.erty3": "val33",
+ "property4": "",
+ },
+ },
+ },
+ },
+ },
+ expectedDimensionKey: "k_e_y",
+ expectedDimensionValue: "id",
  },
  }
  for _, tt := range tests {
@@ -911,6 +966,11 @@ func TestConsumeMetadata(t *testing.T) {
  b, err := ioutil.ReadAll(r.Body)
  assert.NoError(t, err)
 
+ // Test metadata updates are sent onto the right dimensions.
+ dimPair := strings.Split(r.RequestURI, "/")[3:5]
+ assert.Equal(t, tt.expectedDimensionKey, dimPair[0])
+ assert.Equal(t, tt.expectedDimensionValue, dimPair[1])
+
  p := map[string]interface{}{
  "customProperties": map[string]*string{},
  "tags": []string{},
@@ -939,6 +999,7 @@ func TestConsumeMetadata(t *testing.T) {
  Logger: logger,
  SendDelay: 1,
  PropertiesMaxBuffered: 10,
+ MetricsConverter: *converter,
  })
  dimClient.Start()
 

exporter/signalfxexporter/translation/converter.go

@@ -411,3 +411,11 @@ func timestampToSignalFx(ts pdata.Timestamp) int64 {
  // Convert nanosecs to millisecs.
  return int64(ts) / 1e6
 }
+
+func (c *MetricsConverter) ConvertDimension(dim string) string {
+ res := dim
+ if c.metricTranslator != nil {
+ res = c.metricTranslator.translateDimension(dim)
+ }
+ return filterKeyChars(res, c.nonAlphanumericDimChars)
+}

exporter/signalfxexporter/translation/converter_test.go

@@ -912,3 +912,61 @@ func TestNewMetricsConverter(t *testing.T) {
  })
  }
 }
+
+func TestMetricsConverter_ConvertDimension(t *testing.T) {
+ type fields struct {
+ metricTranslator *MetricTranslator
+ nonAlphanumericDimChars string
+ }
+ type args struct {
+ dim string
+ }
+ tests := []struct {
+ name string
+ fields fields
+ args args
+ want string
+ }{
+ {
+ name: "No translations",
+ fields: fields{
+ metricTranslator: nil,
+ nonAlphanumericDimChars: "_-",
+ },
+ args: args{
+ dim: "d.i.m",
+ },
+ want: "d_i_m",
+ },
+ {
+ name: "With translations",
+ fields: fields{
+ metricTranslator: func() *MetricTranslator {
+ t, _ := NewMetricTranslator([]Rule{
+ {
+ Action: ActionRenameDimensionKeys,
+ Mapping: map[string]string{
+ "d.i.m": "di.m",
+ },
+ },
+ }, 0)
+ return t
+ }(),
+ nonAlphanumericDimChars: "_-",
+ },
+ args: args{
+ dim: "d.i.m",
+ },
+ want: "di_m",
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ c, err := NewMetricsConverter(zap.NewNop(), tt.fields.metricTranslator, nil, nil, tt.fields.nonAlphanumericDimChars)
+ require.NoError(t, err)
+ if got := c.ConvertDimension(tt.args.dim); got != tt.want {
+ t.Errorf("ConvertDimension() = %v, want %v", got, tt.want)
+ }
+ })
+ }
+}

exporter/signalfxexporter/translation/translator.go

@@ -588,7 +588,7 @@ func ptToFloatVal(pt *sfxpb.DataPoint) *float64 {
  return &f
 }
 
-func (mp *MetricTranslator) TranslateDimension(orig string) string {
+func (mp *MetricTranslator) translateDimension(orig string) string {
  if translated, ok := mp.dimensionsMap[orig]; ok {
  return translated
  }

exporter/signalfxexporter/translation/translator_test.go

@@ -1923,14 +1923,14 @@ func TestTestTranslateDimension(t *testing.T) {
  }, 1)
  require.NoError(t, err)
 
- assert.Equal(t, "new_dimension", mt.TranslateDimension("old_dimension"))
- assert.Equal(t, "new.dimension", mt.TranslateDimension("old.dimension"))
- assert.Equal(t, "another_dimension", mt.TranslateDimension("another_dimension"))
+ assert.Equal(t, "new_dimension", mt.translateDimension("old_dimension"))
+ assert.Equal(t, "new.dimension", mt.translateDimension("old.dimension"))
+ assert.Equal(t, "another_dimension", mt.translateDimension("another_dimension"))
 
  // Test no rename_dimension_keys translation rule
  mt, err = NewMetricTranslator([]Rule{}, 1)
  require.NoError(t, err)
- assert.Equal(t, "old_dimension", mt.TranslateDimension("old_dimension"))
+ assert.Equal(t, "old_dimension", mt.translateDimension("old_dimension"))
 }
 
 func TestNewCalculateNewMetricErrors(t *testing.T) {