-
Notifications
You must be signed in to change notification settings - Fork 47
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(integrationBehaviour): the integration accept a new parameter to… #63
Conversation
Hi @paologallinaharbur can you please explain the old and new behaviours in the PR description and why it's needed |
… change bahaviour
26970b1
to
91b66e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -0,0 +1,27 @@ | |||
integrations: | |||
- name: nri-prometheus | |||
inventory_source: integration/com.newrelic.prometheus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could remove this and use the default instead. inventory_source
is set by default as integration/nri-prometheus . docs .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why it was set explicitly here, @gsanchezgavier? Removing it here and using the default will change the actual name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no i don't, but inventory is not being used afaik here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just keeping it like this for now, and discuss this with Paolo when he's back. We'll not do a release with it yet, anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
koolio
The nri-prometheus integration now accepts new config/parameters:
Moreover the Viper version has been bumped up since the file passed by the agent is extensionless and only a newer version of Viper supports it and
go mod tidy
has been runOld behaviour:
The process does not terminate after sending the data, therefore once defined the targets they get scraped periodically and the result is sent
Moreover the integration is exposing telemetry data through a prometheus endpoint.
New behaviour
Currently it is just a stub (printing "New Behaviour" and exiting) in order to allow reviewing the change of the main in an easier way.
The new behaviour would transform this integration making it more similar to a onHost integration, therefore it will focus merely on scraping data and processing it. The execution and sending the data will be managed by the agent.
It will output the data on stdOut and exit leveraging the agent to send data and to perform auto-discovery