-
Notifications
You must be signed in to change notification settings - Fork 460
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
[etcd] Use prometheus metricset for etcd v3 metrics #8438
Conversation
change input type improve titles and descriptions
adab794
to
42cab95
Compare
🌐 Coverage report
|
96c28d3
to
5e6ed13
Compare
Can you update the section Compatability highlighting the compatibility metrics ? It may be best to highlight that if the etcd version is 2, dataset named @lalit-satapathy / @SubhrataK , We have three datasets based on V2 API - If etcd version is above 3, by default it supports V3 API. But, It support V2 APIs (by setting ENV variables). So V2 & V3 APIs are supported in etcd version 3 and above. I have tested it with 3.5 version of etcd. The issue here is, V2 APIs provide more metrics when compared to V3 APIs . So, by not supporting V2 API based metrics, customer may loose important insights. @gpop63 please correct me if am wrong. Considering the above points, even when etcd version 2.x is not actively maintained, having V2 API continue to be supported in etcd version 3 and above, should we continue to support datasets leader, self and store in etcd GA version? I think, we should. Like to hear your inputs as well. |
Based on the above comment, I think, we should do the TSDB enablement of all datasets . |
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.
Added review comments
@agithomas The use of etcd v2 is highly discouraged, I think it's not worth spending effort on it. To my knowledge, v2 APIs can be enabled in etcd v3.5, but they are completely removed from v3.6 onwards. Are you suggesting that this data stream should also capture v2 specific metrics from etcd v3 instances where the v2 APIs are enabled? |
I see that 3.6 is in draft state and 3.5 is the latest stable release. Can you share the link to source of info that says V2 APIs will be deprecated from 3.6 ? This will help the decision making process faster |
I am evaluating the benefit of retaining the existing datasets leader, self and store instead of deprecating or removing them as part of GA release. |
|
description: Collecting etcd metrics | ||
title: Collect metrics from etcd v2 instances | ||
description: Collecting metrics etcd v2 metrics | ||
- type: prometheus/metrics |
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.
Is this really necessary as both v2 and v3 endpoint prefix are the same ?
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.
Should we have 1 global hosts
var instead?
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.
Yes, that will serve the purpose as the prefix of both endpoints remain the same.
description: | | ||
Memory allocated bytes as of MemStats Go | ||
- name: go_memstats_alloc.bytes | ||
type: long |
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.
Don't you want to put unit: byte
and similar for field mappings?
@agithomas from the new metrics added, which ones do you think should we add in the dashboard? |
/test |
@ritalwar , can you please take a look at the PR and do a review? Kibana visualisation changes are suggested by me which is pending. You may look at other changes except this. Thanks in advance. |
Co-authored-by: Richa Talwar <[email protected]>
@gpop63 , can you have the panels organised in a way that Panels 1,2,3 are in one row. 10 and 11 in another, 20 and 21 in another ? Please refer to the below mentioned screenshot. Also, please update the package screenshot image as well. |
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!
@ritalwar , can you please take a final look at the PR ? |
packages/etcd/data_stream/metrics/elasticsearch/ingest_pipeline/default.yml
Outdated
Show resolved
Hide resolved
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!
Package etcd - 0.7.0 containing this change is available at https://epr.elastic.co/search?package=etcd |
Overview
Changed the data stream metricset to
prometheus
to ensure we can update and manage the data stream independently of theetcd
beats module releases.Other changes:
TSDB test
Checklist
changelog.yml
file.Author's Checklist
How to test this PR locally
elastic-package stack up -d -v --version 8.12.0-SNAPSHOT
docker-compose.yml
Related issues
Screenshots