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 istiod metricset #21519

Merged
merged 15 commits into from
Oct 13, 2020
Merged

Add istiod metricset #21519

merged 15 commits into from
Oct 13, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Oct 5, 2020 •
edited
Loading

What does this PR do?

This PR adds istiod metricset. It leverages the use_types option.

istiod : light-weight metricset on top of Prometheus to collect metrics from the Prometheus exporter of Istio Daemon

Why is it important?

To be compatible with versions of Istio higher than 1.5.

Related issues

metricbeat-istio-overview

@ChrsMark ChrsMark added in progress Pull request is currently in progress. [zube]: In Progress labels Oct 5, 2020
@ChrsMark ChrsMark self-assigned this Oct 5, 2020
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 5, 2020
@ChrsMark
Add tests
c4a7855 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Fix go.sum
83c88d9 
Signed-off-by: chrismark <[email protected]>
@ChrsMark ChrsMark added the Team:Platforms Label for the Integrations - Platforms team label Oct 5, 2020
@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 5, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented Oct 5, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #21519 updated]

  • Start Time: 2020-10-13T09:06:07.622+0000

  • Duration: 64 min 42 sec

Test stats 🧪

Test Results
Failed 0
Passed 2614
Skipped 576
Total 3190

jsoriano
jsoriano reviewed Oct 6, 2020
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Since this is implemented as a leight-weight module on top of Prometheus should we enable the use_types options?

I started doing this for CockroachDB in #17736, but I found some problems, I don't remember which ones and it seems I didn't write them there 🤦 I only wrote about the lack of formatting options. And there was also the problem with duplicated metrics.

I think that the best option for this would be to give it a try, and see if it works well for this case. I should also give another try to CockroachDB.

x-pack/metricbeat/module/istio/_meta/config.reference.yml Show resolved Hide resolved
@ChrsMark
Copy link
Member Author

ChrsMark commented Oct 6, 2020

Thanks for the review @jsoriano ! Something relevant to use_types that might makes the usage of it for now discouraging: #21507 (comment)

jsoriano
jsoriano previously approved these changes Oct 6, 2020
View reviewed changes
@ChrsMark
Use types and add dashboard
17a702d 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
fix docs
e3bc87b 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

ChrsMark commented Oct 7, 2020

I tried to leverage use_types and seems to work fine. I also added a dashboard for it. @jsoriano @exekias let me know what you think.

@jsoriano
Copy link
Member

jsoriano commented Oct 7, 2020

I tried to leverage use_types and seems to work fine.

Nice! Could you update the expected json file? It doesn't seem to be using the histogram type.

@ChrsMark
Update data.json
bf40b70 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
fix fields back
d978130 
Signed-off-by: chrismark <[email protected]>
@ChrsMark
Copy link
Member Author

ChrsMark commented Oct 7, 2020

Expected json file was updated.

@ChrsMark
Fix index reference
7d7b439 
Signed-off-by: chrismark <[email protected]>
@ChrsMark ChrsMark changed the title Add istiod metricset Add istiod and proxy metricset Oct 8, 2020
@ChrsMark ChrsMark changed the title Add istiod and proxy metricset Add istiod and proxy metricsets Oct 8, 2020
@ChrsMark
Copy link
Member Author

This one has been updated a little bit so as to add proxy metricset along with an extra dashboard.
@jsoriano @exekias could we go for another review round on this please?

@jsoriano
Copy link
Member

This one has been updated a little bit so as to add proxy metricset along with an extra dashboard.

@ChrsMark the proxy dashboard would need an update on the titles so [Metricbeat istio] is not visible, and there seems to be a linting issue too.

Regarding the dashboard for proxy, the duration and size graphs using percentiles? I find confusing to mix in the same graph the percentiles of different endpoints, with several endpoints the graph will be bloated. I think that it would be better to show a) multiple percentiles but with the an aggregation of all the requests to all endpoints, so each percentile appears only once in the graph, or b) an only percentile (e.g. the 99th one) for the top endpoints.

The istiod was almost ready to be merged, wdyt about keeping this PR only for this metricset, and open a different one for the proxy metricset?

@jsoriano jsoriano dismissed their stale review October 12, 2020 19:52

More changes introduced.

@ChrsMark
Copy link
Member Author

The istiod was almost ready to be merged, wdyt about keeping this PR only for this metricset, and open a different one for the proxy metricset?

👍 , moved things back to keep only the istiod metricset for now.

Will keep option (b) for Proxy in the upcoming PR.

@ChrsMark ChrsMark requested a review from jsoriano October 13, 2020 09:08
@jsoriano jsoriano changed the title Add istiod and proxy metricsets Add istiod metricset Oct 13, 2020
@ChrsMark ChrsMark merged commit 5536fb6 into elastic:master Oct 13, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Oct 13, 2020
@ChrsMark
Add istiod metricset (elastic#21519)
d11251a 
(cherry picked from commit 5536fb6)
@ChrsMark ChrsMark mentioned this pull request Oct 13, 2020
Merged
v1v added a commit to v1v/beats that referenced this pull request Oct 13, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-10
aa4bf15 
* upstream/master: (127 commits)
  Update obs app links (elastic#21682)
  fix: update fleet test suite name (elastic#21738)
  Remove dot from file.extension value in Auditbeat FIM (elastic#21644)
  Fix leaks with metadata processors (elastic#16349)
  Add istiod metricset (elastic#21519)
  [Ingest Manager] Change Sync/Close call order (elastic#21735)
  [Ingest Manager] Syncing unpacked files (elastic#21706)
  Fix concurrent map read and write in socket dataset (elastic#21690)
  Fix conditional coding to remove seccomp info from Winlogbeat (elastic#21652)
  [Elastic Agent] Fix issue where inputs without processors defined would panic (elastic#21628)
  Add configuration of filestream input (elastic#21565)
  libbeat/logp: introduce Logger.WithOptions (elastic#21671)
  Make o365audit input cancellable (elastic#21647)
  fix: remove extra curly brace in script (elastic#21692)
  [Winlogbeat] Remove brittle configuration validation from wineventlog (elastic#21593)
  Fix function that parses from/to/contact headers (elastic#21672)
  [CI] Support Windows-2016 in pipeline 2.0 (elastic#21337)
  Skip publisher flaky tests (elastic#21657)
  backport: add 7.10 branch (elastic#21635)
  [CI: Packaging] fix: push ubi8 images too (elastic#21621)
  ...
ChrsMark added a commit that referenced this pull request Oct 13, 2020
@ChrsMark
Add istiod metricset (#21519) (#21737)
ab5a7fe 
(cherry picked from commit 5536fb6)
@ChrsMark ChrsMark mentioned this pull request Oct 13, 2020
Merged
v1v added a commit to v1v/beats that referenced this pull request Oct 14, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/download-…
2bc6d54 
…dependencies-goals

* upstream/master: (46 commits)
  Use badger code from upstream repository (elastic#21705)
  Disable writes sync in persistent cache (elastic#21754)
  Make API address and Shard ID required in Cloud Foundry settings (elastic#21759)
  [CI] Support skip-ci label (elastic#21377)
  Increase recommended memory when deploying in Cloud foundry (elastic#21755)
  typofix for dns timeout configuration option (elastic#21069)
  chore: create CI artifacts for DEV usage (elastic#21645)
  [Ingest Manager] Atomic installed forgotten changelog elastic#21780
  [Ingest Manager] Agent atomic installer (elastic#21745)
  Add missing configuration annotations (elastic#21736)
  [Filebeat] Add check for context.DeadlineExceeded error (elastic#21732)
  Remove kafka partition ISR from Metricbeat docs (elastic#21709)
  Skip flaky test with oauth2 config in httpjson input (elastic#21749)
  Fix for azure retrieve resource by ids (elastic#21711)
  Update obs app links (elastic#21682)
  fix: update fleet test suite name (elastic#21738)
  Remove dot from file.extension value in Auditbeat FIM (elastic#21644)
  Fix leaks with metadata processors (elastic#16349)
  Add istiod metricset (elastic#21519)
  [Ingest Manager] Change Sync/Close call order (elastic#21735)
  ...
This was referenced Oct 15, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees

@ChrsMark ChrsMark

Labels
hacktoberfest-accepted in progress Pull request is currently in progress. Team:Platforms Label for the Integrations - Platforms team v7.11.0 [zube]: In Progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement istiod metricset
3 participants
@ChrsMark @elasticmachine @jsoriano