Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add spec files for entity modeling #68

Merged
merged 6 commits into from
Sep 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.DS_Store
.idea
.vscode
*.log
tmp/
bin/
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ require (
github.com/stretchr/objx v0.1.2-0.20180626195558-9e1dfc121bca // indirect
github.com/stretchr/testify v1.6.1
golang.org/x/crypto v0.0.0-20200220183623-bac4c82f6975 // indirect
gopkg.in/yaml.v2 v2.2.8
gsanchezgavier marked this conversation as resolved.
Show resolved Hide resolved
k8s.io/api v0.16.10
k8s.io/apimachinery v0.16.10
k8s.io/client-go v0.15.12
Expand Down
146 changes: 146 additions & 0 deletions internal/integration/spec.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
// Package integration ...
// Copyright 2019 New Relic Corporation. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
package integration

import (
"fmt"
"io/ioutil"
"path"
"regexp"
"strings"

"github.com/sirupsen/logrus"

yaml "gopkg.in/yaml.v2"
)

const fileNameMatcher = `^prometheus_.*\.ya?ml$`

// Specs contains all the services specs mapped with the service name
type Specs struct {
SpecsByName map[string]SpecDef
}

// SpecDef contains the rules to group metrics into entities
type SpecDef struct {
Service string `yaml:"service"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the some doubt when writing the prometheus parser, I do not know if we can assume we have only one service.
As we discussed today we might have a single exporter returning two (or more "services"). In that case either we will have multiple file or we need to change the structure to something like:

type SpecDef struct {
    Provider string      `yaml:"provider"`
    Service  []Service `yaml:"service"`
}

type Service struct {
	ServiceName  string           `yaml:"service"`
	Entities            []EntityDef  `yaml:"entities"`
}

In this case we would not follow cloud integration structure

Copy link
Contributor Author

@gsanchezgavier gsanchezgavier Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for bring this up. If for some reason the same exporter exposes metrics from different services we can define a spec file for each of the service and keep the same structure.
The service will be picked from the metric prefix and treated as defined in the corresponding spec file.
I'm thinking about that this solution could also work for the exporter related metrics like go_ or the ones related to the host where the exporter runs like process_

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsanchezgavier To be clear, you're saying that in the case of 2 or more services for the same exporter we will create a file for each one? If that is your idea, I just worry about how it will impact external contributors, especially customers. It may be difficult to explain that, even with documentation, and the more files we have the harder it gets to contribute. We should really try to reduce the number of files required to make an exporter work.
I'm not sure we need to follow the exact same structure of the cloud integrations, especially if it doesn't work well for us.
Just my 2cts

Copy link
Member

@paologallinaharbur paologallinaharbur Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should try as much as possible to have one file per integration.

Either we follow the cloud integration approach adding more services with the ---
ES:

provider: prometheus
service: ravendb1
display_name: Raven Db
entities:
[...]
---
provider: prometheus
service: ravendb2
display_name: Raven Db
entities:
[...]

Or we can change the structure allowing arrays of services in one yaml. We will need to change a bit the automatic tool but then we will be free to diverge when needed

ES:

provider: prometheus
display_name: Raven Db
services:
  - ravendb1
     entities:
     [...]
  - ravendb2
     entities:
     [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morecar needs your opinion here. I think cloud int. also splits some specs files for some integrations.
I also want to mention that i think having 2 different services in one exporter don't looks like a good prometheus best practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may need to look into some more exporters to get an idea of the number of "services" that they expose.
On the other hand we can just assume that 1 exporter = 1 service. For example Redis has metrics that start with very different "prefixes": redis_ , process_ , go_ . If we ignore the go prefix (which the exporter can be configured to not return), the metrics all belong to the Redis service, just with different entities. I'm not sure about other exporters but I'm guessing it may be the same.

In summary, I would not make services an array. If we find we need more than one service for one exporter we can use the first option, having multiple services on the same file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

Entities []EntityDef `yaml:"entities"`
}

// EntityDef has info related to each entity
type EntityDef struct {
Type string `yaml:"name"`
gsanchezgavier marked this conversation as resolved.
Show resolved Hide resolved
Properties PropertiesDef `yaml:"properties"`
Metrics []MetricDef `yaml:"metrics"`
}

// PropertiesDef defines the dimension used to get entity names
type PropertiesDef struct {
Dimensions []string `yaml:"dimensions"`
}

// MetricDef contains metrics definitions
type MetricDef struct {
Name string `yaml:"provider_name"`
}

// LoadSpecFiles loads all service spec files named like "prometheus_*.yml" that are in the filesPath
func LoadSpecFiles(filesPath string) (Specs, error) {
specs := Specs{SpecsByName: make(map[string]SpecDef)}
var files []string

filesInPath, err := ioutil.ReadDir(filesPath)
if err != nil {
return specs, err
}
for _, f := range filesInPath {
if ok, _ := regexp.MatchString(fileNameMatcher, f.Name()); ok {
files = append(files, path.Join(filesPath, f.Name()))
}
}

for _, file := range files {
f, err := ioutil.ReadFile(file)
if err != nil {
logrus.Errorf("fail to read service spec file %s: %s ", file, err)
continue
}

var sd SpecDef
err = yaml.Unmarshal(f, &sd)
if err != nil {
logrus.Errorf("fail parse service spec file %s: %s", file, err)
continue
}
logrus.Debugf("spec file loaded for service:%s", sd.Service)
specs.SpecsByName[sd.Service] = sd
}

return specs, nil
}

// getEntity returns entity name and type of the metric based on the spec configuration defined for the service.
// conditions for the metric:
// - metric.name has to start a prefix that matches with a service from the spec files
// - metric.name has to be defined in one of the entities of the spec file
// - if dimension has been specified for the entity, the metric need to have all of them.
// - metrics that belongs to entities with no dimension specified will share the same name
func (s *Specs) getEntity(m Metric) (entityName string, entityType string, err error) {
ardias marked this conversation as resolved.
Show resolved Hide resolved
spec, err := s.findSpec(m.name)
if err != nil {
return "", "", err
}

e, ok := spec.findEntity(m.name)
if !ok {
return "", "", fmt.Errorf("metric: %s is not defined in service:%s", m.name, spec.Service)
}

entityType = strings.Title(spec.Service) + strings.Title(e.Type)

entityName = e.Type

for _, d := range e.Properties.Dimensions {
var val interface{}
var ok bool
// the metric needs all the dimensions defined to avoid entity name collision
if val, ok = m.attributes[d]; !ok {
return "", "", fmt.Errorf("dimension %s not found in metric %s", d, m.name)
}
// entity name will be composed by the value of the dimensions defined for the entity in order
entityName = entityName + ":" + fmt.Sprintf("%v", val)
}

return entityName, entityType, nil
}

// findSpec parses the metric name to extract the service and resturns the spec definition that matches
func (s *Specs) findSpec(metricName string) (SpecDef, error) {
var spec SpecDef

res := strings.SplitN(metricName, "_", 2)
if len(res) < 2 {
return spec, fmt.Errorf("metric: %s has no suffix to identify the entity", metricName)
}
serviceName := res[0]

var ok bool
if spec, ok = s.SpecsByName[serviceName]; !ok {
return spec, fmt.Errorf("no spec files for service: %s", serviceName)
}

return spec, nil
}

// findEntity returns the entity where metricName is defined
func (s *SpecDef) findEntity(metricName string) (EntityDef, bool) {
for _, e := range s.Entities {
for _, em := range e.Metrics {
if metricName == em.Name {
return e, true
}
}
}
return EntityDef{}, false
}
152 changes: 152 additions & 0 deletions internal/integration/spec_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
// Copyright 2019 New Relic Corporation. All rights reserved.
// SPDX-License-Identifier: Apache-2.0
package integration

import (
"testing"

"github.com/newrelic/nri-prometheus/internal/pkg/labels"
"github.com/stretchr/testify/assert"
)

func TestLoadSpecFilesOk(t *testing.T) {
specs, err := LoadSpecFiles("./test/")
assert.NoError(t, err)
assert.Contains(t, specs.SpecsByName, "ibmmq")
assert.Contains(t, specs.SpecsByName, "ravendb")
assert.Contains(t, specs.SpecsByName, "redis")
}
func TestLoadSpecFilesNoFiles(t *testing.T) {
specs, err := LoadSpecFiles(".")
assert.NoError(t, err)
assert.Len(t, specs.SpecsByName, 0)
}

func TestSpecs_getEntity(t *testing.T) {

specs, err := LoadSpecFiles("./test/")
assert.NoError(t, err)

type fields struct {
SpecsByName map[string]SpecDef
}
type args struct {
m Metric
}
tests := []struct {
name string
fields fields
args args
wantEntityName string
wantEntityType string
wantErr bool
}{
{
name: "matchEntity",
fields: fields{specs.SpecsByName},
args: args{
Metric{
name: "ravendb_database_document_put_bytes_total",
attributes: labels.Set{
"database": "test",
},
}},
wantEntityName: "database:test",
wantEntityType: "RavendbDatabase",
wantErr: false,
},
{
name: "matchEntityWithoutDimensions",
fields: fields{specs.SpecsByName},
args: args{
Metric{
name: "ravendb_document_put_bytes_total",
attributes: labels.Set{},
}},
wantEntityName: "node",
wantEntityType: "RavendbNode",
wantErr: false,
},
{
name: "matchEntityConcatenatedName",
fields: fields{specs.SpecsByName},
args: args{
Metric{
name: "ravendb_testentity_document_put_bytes_total",
attributes: labels.Set{
"dim1": "first",
"dim2": "second",
},
}},
wantEntityName: "testentity:first:second",
wantEntityType: "RavendbTestentity",
wantErr: false,
},
{
name: "redisEntityMetric1",
fields: fields{specs.SpecsByName},
args: args{
Metric{
name: "redis_commands_duration_seconds_total",
attributes: labels.Set{},
}},
wantEntityName: "instance",
wantEntityType: "RedisInstance",
wantErr: false,
},
{
name: "redisEntityMetric2",
fields: fields{specs.SpecsByName},
args: args{
Metric{
name: "redis_repl_backlog_is_active",
attributes: labels.Set{},
}},
wantEntityName: "instance",
wantEntityType: "RedisInstance",
wantErr: false,
},
{
name: "missingDimentions",
fields: fields{specs.SpecsByName},
args: args{Metric{name: "ravendb_database_document_put_bytes_total"}},
wantErr: true,
},
{
name: "serviceNotDefined",
fields: fields{specs.SpecsByName},
args: args{Metric{name: "service_metric_undefined"}},
wantErr: true,
},
{
name: "metricNotDefined",
fields: fields{specs.SpecsByName},
args: args{Metric{name: "ravendb_metric_undefined"}},
wantErr: true,
},
{
name: "shortMetricName",
fields: fields{specs.SpecsByName},
args: args{Metric{name: "shortname"}},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := &Specs{
SpecsByName: tt.fields.SpecsByName,
}
gotEntityName, gotEntityType, err := s.getEntity(tt.args.m)
if (err != nil) != tt.wantErr {
t.Errorf("Specs.getEntity() error = %v, wantErr %v", err, tt.wantErr)
return
}
if gotEntityName != tt.wantEntityName {
t.Errorf("Specs.getEntity() gotEntityName = %v, want %v", gotEntityName, tt.wantEntityName)
}
if gotEntityType != tt.wantEntityType {
t.Errorf("Specs.getEntity() gotEntityType = %v, want %v", gotEntityType, tt.wantEntityType)
}
})
}
}
20 changes: 20 additions & 0 deletions internal/integration/test/prometheus_ibmmq.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
service: ibmmq
display_name: IBM MQ
entities:
- name: qmgr
display_name: Queue Manager
properties:
dimensions: [ qmgr, platform ]
metrics:
- provider_name: ibmmq_qmgr_alter_durable_subscription_count
description: Alter durable subscription count
unit: Count
- provider_name: ibmmq_qmgr_channel_initiator_status
description: Channel Initiator Status
unit: Gauge
- provider_name: ibmmq_qmgr_commit_count
description: Commit count
unit: Count
- provider_name: go_goroutines
description: Number of goroutines that currently exist.
unit: gauge
35 changes: 35 additions & 0 deletions internal/integration/test/prometheus_ravendb.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
service: ravendb
display_name: Raven Db
entities:
- name: node
display_name: RavenDb Node
metrics:
- provider_name: ravendb_document_put_bytes_total
description: Server-wide document put bytes
unit: Count
- provider_name: ravendb_is_leader
description: If 1, then node is the cluster leader, otherwise 0
unit: Gauge
- name: database
display_name: Database
properties:
dimensions: [ database ]
metrics:
- provider_name: ravendb_database_document_put_bytes_total
description: Database document put bytes
unit: Count
- provider_name: ravendb_database_document_put_total
description: Database document puts count
unit: Count
- name: testentity
display_name: testEntity
properties:
dimensions: [ dim1, dim2 ]
metrics:
- provider_name: ravendb_testentity_document_put_bytes_total
description: Database document put bytes
unit: Count
- provider_name: ravendb_testentity_document_put_total
description: Database document puts count
unit: Count

13 changes: 13 additions & 0 deletions internal/integration/test/prometheus_redis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
service: redis
display_name: Redis
entities:
- name: instance
display_name: Redis Instance
metrics:
- provider_name: redis_commands_duration_seconds_total
description: Total amount of time in seconds spent per command
unit: Counter
- provider_name: redis_repl_backlog_is_active
description: repl_backlog_is_active metric
unit: Gauge