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

Add multiplier option to prometheus helper #10994

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

jsoriano
Copy link
Member

Prometheus histograms can contain buckets with keys in decimal
representation. They create events with dots in the field names, what
can be problematic when stored in Elasticsearch as they will create
sub-objects.

Add a new option to allow to store these keys in different units, so
precission is kept but in a different order of magnitude.

Prometheus histograms can contain buckets with keys in decimal
representation. They create events with dots in the field names, what
can be problematic when stored in Elasticsearch as they will create
sub-objects.

Add a new option to allow to store these keys in different units, so
precission is kept but in a different order of magnitude.
@jsoriano jsoriano added enhancement module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 28, 2019
@jsoriano jsoriano self-assigned this Feb 28, 2019
@jsoriano jsoriano requested a review from a team as a code owner February 28, 2019 15:51
@jsoriano jsoriano requested review from a team and removed request for a team February 28, 2019 18:13
@jsoriano
Copy link
Member Author

jenkins, test this again please

@ChrsMark
Copy link
Member

ChrsMark commented Mar 6, 2019

Hello, any update on this?

Copy link
Contributor

@sayden sayden left a comment

Choose a reason for hiding this comment

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

I'm fine with the code like this. Just left some comments if they give you some ideas :)

}

// Process will multiply the bucket labels if it is an histogram with numeric labels
func (o opMultiplyBuckets) Process(field string, value interface{}, labels common.MapStr) (string, interface{}, common.MapStr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for not using pointers here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I didn't think about this, I just followed what was being done for other options.

In any case I wouldn't change it now, we can consider doing it for all options in the future.

@@ -57,6 +57,13 @@ func OpLowercaseValue() MetricOption {
return opLowercaseValue{}
}

// OpMultiplyBuckets multiplies bucket labels in histograms, useful to change units
func OpMultiplyBuckets(multiplier float64) MetricOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this NewOpMultiplyBucketsOption unless we have some internal convention for this

Copy link
Member Author

@jsoriano jsoriano Mar 6, 2019

Choose a reason for hiding this comment

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

I don't have an strong opinion on this but yes, I am following the naming schemas used here.

Take into account that they are not so bad as they are used as options in prometheus metrics:

Metrics: map[string]MetricMap{
    "histogram_decimal_metric": Metric("histogram.metric", OpMultiplyBuckets(1000))
}

@jsoriano
Copy link
Member Author

jsoriano commented Mar 6, 2019

Failing test is not related, and green in travis, merging.

@jsoriano jsoriano merged commit 5c734b9 into elastic:master Mar 6, 2019
@jsoriano jsoriano deleted the prometheus-helper-modifiers branch March 6, 2019 12:25
@ChrsMark
Copy link
Member

ChrsMark commented Mar 6, 2019

hey @jsoriano, trying to update #10585 two small questions occurred to me:

  1. Shouldn't we multiply the sum field of the histogram too, since we have histogram's values to be scaled?
  2. Why not to scale all the numeric values and only floats? If we have a histogram with 3 floats as keys and 2 integers then converting only the floats will result into a wrongly scaled histogram, right?

Sorry for not finding earlier. 😞
If we agree on those I can include them on #10585

@jsoriano
Copy link
Member Author

jsoriano commented Mar 6, 2019

@ChrsMark oh, you are totally right about the sum, good catch, I have opened a PR to fix this #11111

Regarding the other question, it should scale anything that parses as float, what should include any number, actually the test case added here includes an integer (1).

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants