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

Remove dataType field of query metricset #17383

Merged
merged 5 commits into from
Apr 2, 2020
Merged

Remove dataType field of query metricset #17383

merged 5 commits into from
Apr 2, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Apr 1, 2020 •
edited by kaiyan-sheng
Loading

What does this PR do?

This PRs moves prometheus.query.dataType field to prometheus.labels.dataType.

Why is it important?

prometheus.query.dataType is of type string however the mapping of prometheus.query.* is for float values (and actually is used to store metrics). This makes it unable to index events in Elasticsearch. Hence, prometheus.query.dataType should be moved under labels which are mapped as strings.

closes #17401

@ChrsMark ChrsMark added bug needs_backport PR is waiting to be backported to other branches. v7.7.0 Team:Platforms Label for the Integrations - Platforms team v7.8.0 labels Apr 1, 2020
@ChrsMark ChrsMark requested review from exekias and a team April 1, 2020 08:13
@ChrsMark ChrsMark self-assigned this Apr 1, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@ChrsMark ChrsMark changed the title Move dataType field in prometheus labels Move dataType field of query metricset under prometheus.labels Apr 1, 2020
exekias
exekias reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Can we put it somewhere else? This is not exactly a label. it would be prometheus.datatype for instance.

Also I wonder if we really need to store this, what's the benefit?

@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 1, 2020

Can we put it somewhere else? This is not exactly a label. it would be prometheus.datatype for instance.

The logic of adding this as a label is to follow the Prometheus approach of storing only numbers under metrics and everything that is string to be added as label.

Also I wonder if we really need to store this, what's the benefit?

The only benefit I see is for the users to be able to better watch the types of the queries they have. I have no strong preference in keeping this, we can discard it.

@exekias
Copy link
Contributor

exekias commented Apr 1, 2020

+1 to remove it until we find a use case for it. We can always add it afterwards

@ChrsMark ChrsMark changed the title Move dataType field of query metricset under prometheus.labels Remove dataType field of query metricset Apr 1, 2020
kaiyan-sheng
kaiyan-sheng reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

ResultType defined in structs can be removed as well? For example: https://github.com/elastic/beats/blob/master/metricbeat/module/prometheus/query/data.go#L48 and https://github.com/elastic/beats/blob/master/metricbeat/module/prometheus/query/data.go#L66

@ChrsMark ChrsMark requested a review from kaiyan-sheng April 1, 2020 18:07
@ChrsMark
Copy link
Member Author

ChrsMark commented Apr 1, 2020

Good catch @kaiyan-sheng , thanks!

@ChrsMark
fmt
19118e2 
Signed-off-by: chrismark <[email protected]>
kaiyan-sheng
kaiyan-sheng approved these changes Apr 1, 2020
View reviewed changes
Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

LGMT! Thanks!

@ChrsMark ChrsMark merged commit 495595d into elastic:master Apr 2, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 2, 2020
@ChrsMark
Remove dataType field of query metricset (elastic#17383)
0cad847 
(cherry picked from commit 495595d)
@ChrsMark ChrsMark mentioned this pull request Apr 2, 2020
Merged
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Apr 2, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Apr 2, 2020
@ChrsMark
Remove dataType field of query metricset (elastic#17383)
8d56de1 
(cherry picked from commit 495595d)
@ChrsMark ChrsMark mentioned this pull request Apr 2, 2020
Merged
ChrsMark added a commit that referenced this pull request Apr 2, 2020
@ChrsMark
Remove dataType field of query metricset (#17383) (#17427)
e5b35b8 
(cherry picked from commit 495595d)
ChrsMark added a commit that referenced this pull request Apr 2, 2020
@ChrsMark
Remove dataType field of query metricset (#17383) (#17426)
407cd5a 
(cherry picked from commit 495595d)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
@ChrsMark
Remove dataType field of query metricset (elastic#17383) (elastic#17426)
8d3dfa2 
(cherry picked from commit deba29e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaiyan-sheng kaiyan-sheng kaiyan-sheng approved these changes

@exekias exekias exekias approved these changes

Assignees

@ChrsMark ChrsMark

Labels
bug Team:Platforms Label for the Integrations - Platforms team v7.7.0 v7.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Metricbeat] Failed to parse field [prometheus.query.dataType]
5 participants
@ChrsMark @elasticmachine @exekias @kaiyan-sheng @andresrc