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.

Sign up for GitHub

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

Jump to bottom

Add Namespace to prometheus helper mappings #11424

Merged
merged 2 commits into from
Mar 25, 2019
Merged

Add Namespace to prometheus helper mappings #11424

merged 2 commits into from
Mar 25, 2019

Conversation

odacremolbap
Copy link
Contributor

@odacremolbap odacremolbap commented Mar 25, 2019 •
edited by alvarolobato
Loading

Add Namespace to prometheus.MetricsMappings
When a namespace is specified it will propagated to the mb.Eventobject

Fixes #11423

@odacremolbap odacremolbap added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 25, 2019
@odacremolbap odacremolbap requested review from jsoriano and ruflin March 25, 2019 12:32
@odacremolbap odacremolbap requested a review from a team as a code owner March 25, 2019 12:32
@odacremolbap odacremolbap changed the title add Namespace to prometheus helper mappings Add Namespace to prometheus helper mappings Mar 25, 2019
@odacremolbap odacremolbap mentioned this pull request Mar 25, 2019
Merged
ruflin
ruflin reviewed Mar 25, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Probably also worth updating the developer changelog with this addition.

metricbeat/helper/prometheus/prometheus.go
r.Event(mb.Event{MetricSetFields: event})
r.Event(mb.Event{
MetricSetFields: event,
Namespace: mapping.Namespace,
Copy link
Member

Choose a reason for hiding this comment

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

Should we set it only in here if it's not empty? Not sure if otherwise it could have some side effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a string, so empty val is "" --> there should be no issue.

Copy link
Member

Choose a reason for hiding this comment

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

Just checked the code where the event is generated and indeed we check for ""

@odacremolbap odacremolbap requested a review from ruflin March 25, 2019 13:55
@odacremolbap odacremolbap merged commit 4267241 into elastic:master Mar 25, 2019
@odacremolbap odacremolbap deleted the task/add-namespaced-mappings-for-prometheus branch March 25, 2019 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@jsoriano jsoriano Awaiting requested review from jsoriano

Assignees

@odacremolbap odacremolbap

Labels
Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@odacremolbap @ruflin @exekias @alvarolobato