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

[Ceph] Add Integration Package with OSD Pool Stats data stream #5128

Merged

Conversation

harnish-elastic
Copy link
Contributor

@harnish-elastic harnish-elastic commented Jan 27, 2023

  • Enhancement

What does this PR do?

  • Added 1 data stream (OSD Pool Stats metrics).
  • Added data collection logic for the data streams.
  • Added the ingest pipeline for the data streams.
  • Mapped fields according to the ECS schema and added Fields metadata in the appropriate yaml files.
  • Added dashboards and visualizations.
  • Added system test cases for the data stream.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

  • Clone integrations repo.
  • Install elastic-package locally.
  • Start elastic stack using elastic-package.
  • Move to integrations/packages/ceph directory.
  • Run the following command to run tests.
    elastic-package test

Related issues

@kush-elastic kush-elastic self-requested a review January 27, 2023 11:15
@kush-elastic kush-elastic added enhancement New feature or request Integration:Ceph Team:Service-Integrations Label for the Service Integrations team labels Jan 27, 2023
@elasticmachine
Copy link

elasticmachine commented Jan 27, 2023

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-06T13:31:40.281+0000

  • Duration: 16 min 45 sec

Test stats 🧪

Test Results
Failed 0
Passed 15
Skipped 0
Total 15

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@kush-elastic
Copy link
Collaborator

/test

@elasticmachine
Copy link

elasticmachine commented Jan 27, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (3/3) 💚
Files 100.0% (3/3) 💚
Classes 100.0% (3/3) 💚
Methods 100.0% (24/24) 💚
Lines 92.547% (149/161) 👍 1.057
Conditionals 100.0% (0/0) 💚

@harnish-elastic harnish-elastic marked this pull request as ready for review January 27, 2023 11:36
Copy link
Collaborator

@kush-elastic kush-elastic left a comment

Choose a reason for hiding this comment

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

LGTM!

packages/ceph/data_stream/osd_pool_stats/fields/fields.yml Outdated Show resolved Hide resolved
fields:
- name: bytes
type: long
description: Number of client I/O rates write bytes per second.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change it to write rates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, Do we need to update description as Number of client I/O rates write bytes per second. -> Number of client I/O write rates bytes per second.?

Copy link
Contributor

Choose a reason for hiding this comment

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

Number of client I/O write rates in bytes per second

Copy link
Contributor Author

@harnish-elastic harnish-elastic Jan 30, 2023

Choose a reason for hiding this comment

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

Got it. Will update wherever applicable. Thanks!

description: Number of client I/O rates write bytes per second.
metric_type: gauge
unit: byte
- name: operation.count
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to use operation_count as there exist no more than 1 fields under operation group

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are following Elastic best practice while doing field mappings. Hence we have added count suffix wherever it's applicable. Final json structure would be like ceph.osd_pool_stats.client_io_rate.write.operation.count. Let me know if it looks good to you!

- name: id
type: long
description: Pool ID.
- name: name
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to rename as pool_name and pool_id

- name: read
type: group
fields:
- name: bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be best to change to write.byte to write.rate_per_sec , read.byte to read.rate_per_sec. This is following the convetion in oracle.sysmetric fields mapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on your suggestions, final output will be ceph.osd_pool_stats.client_io_rate.write.rate_per_sec that is making rate redundant. I guess currently it is the best structure that provides the IO rates and in group only. So readability also increase as per the current json event structure. I see in oracle.sysmetric no json structure is form eventhough we can. According to me, The current best json structure for the user is ceph.osd_pool_stats.client_io_rate.write.bytes and same is applicable for the field read.bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

You may consider changing from ceph.osd_pool_stats.client_io_rate -> ceph.osd_pool_stats.client_io, if there are no constraints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of updating ceph.osd_pool_stats.client_io_rate -> ceph.osd_pool_stats.client_io, We can update ceph.osd_pool_stats.client_io_rate -> ceph.osd_pool_stats.client_io_rate_per_sec, Because all the client_io_rate are descripted as per_sec. So the updating field mapping will be like ceph.osd_pool_stats.client_io_rate_per_sec.read.bytes. WDYT?

Copy link
Contributor

@agithomas agithomas Jan 30, 2023

Choose a reason for hiding this comment

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

Go with removing operation. from everywhere. This would solve all the problems.

Every IO is an operation. So, no need to use it anywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep rest as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure we should not keep the operation. in field mapping? I guess it is not creating any problems based on current structure. I/O can be operation, device, program anything. According to me its seems fine to callout operation separately.

Copy link
Contributor

@agithomas agithomas Jan 30, 2023

Choose a reason for hiding this comment

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

Yes, please go ahead removing it

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM

@harnish-elastic harnish-elastic requested a review from a team as a code owner February 6, 2023 13:31
@kush-elastic kush-elastic merged commit f273bae into elastic:main Feb 6, 2023
@elasticmachine
Copy link

Package ceph - 0.3.0 containing this change is available at https://epr.elastic.co/search?package=ceph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:Ceph Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants