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

Conversation

gsanchezgavier
Copy link
Contributor

This PR adds the spec files model that will be use to compose entities with the metrics based on the file definition.

The functions added here will be used by the integration when running with the Agent and using sdk emitter.

All spec files will be located in a specific folder and pomi will load them every execution.

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2020

CLA assistant check
All committers have signed the CLA.

@gsanchezgavier gsanchezgavier force-pushed the gsanchez/entity_modeling branch from 5563f6f to b20b0a9 Compare August 26, 2020 08:31
go.mod Show resolved Hide resolved
internal/integration/spec.go Outdated Show resolved Hide resolved
@gsanchezgavier gsanchezgavier force-pushed the gsanchez/entity_modeling branch from 09bd344 to f3d0532 Compare August 28, 2020 13:24
// SpecDef contains the rules to group metrics into entities
type SpecDef struct {
Provider string `yaml:"provider"`
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

ardias
ardias previously approved these changes Sep 1, 2020
Copy link
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

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

LGTM.

attributes: labels.Set{},
}},
wantEntityName: "node",
wantEntityType: "PrometheusRavendbNode",
Copy link
Member

Choose a reason for hiding this comment

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

Are we going to remove Prometheus prefix from entity name in next iteration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i'll do it

@gsanchezgavier gsanchezgavier force-pushed the gsanchez/entity_modeling branch from 53f080d to 5e8aae9 Compare September 2, 2020 07:22
@gsanchezgavier
Copy link
Contributor Author

rebased from main and remove provider prefix

Copy link
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

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

Thanks!

@gsanchezgavier gsanchezgavier merged commit 5ff9dea into main Sep 2, 2020
@gsanchezgavier gsanchezgavier deleted the gsanchez/entity_modeling branch September 2, 2020 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants