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 cassandra package #1745

Merged
merged 7 commits into from
Oct 6, 2021
Merged

Conversation

kush-elastic
Copy link
Collaborator

@kush-elastic kush-elastic commented Sep 21, 2021

What does this PR do?

  • This PR is draft PR for suggestions and reviewing cassandra integration for log and metrics 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.
  • If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

How to test this PR locally

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

Related issues

Screenshots

Tests

Kibana-Log

Kibana-Metrics

Metrics Cassandra  Overview - Elastic(updated1)_pages-to-jpg-0001

Logs Cassandra  System Logs - Elastic_page-0001

@elasticmachine
Copy link

elasticmachine commented Sep 21, 2021

💚 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: 2021-10-05T12:32:00.742+0000

  • Duration: 13 min 8 sec

  • Commit: 58a987e

Test stats 🧪

Test Results
Failed 0
Passed 33
Skipped 0
Total 33

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@mtojek
Copy link
Contributor

mtojek commented Sep 22, 2021

/test

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

First round of reviews, left few comments on the design.

@kush-elastic kush-elastic changed the title Add cassandra package and log data-stream Add cassandra package Sep 30, 2021

ENV JMX_REMOTE=yes JOLOKIA_ENABLED=yes

COPY docker-entrypoint-initdb.d /docker-entrypoint-initdb.d
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe add a comment why do we need a custom docker entrypoint

@@ -0,0 +1,4 @@
variants:
v3.11.11:
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try with older versions to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i have tested integration with:
2.2
3.0
3.11
4.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but could you define it here to see if the integration works?

It can be done once this PR is merged (non-blocking).

multi: false
required: false
show_user: true
default: ffa
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is it the default Cassandra user/password: ffa/ffa? If it isn't, can we change it to something "really" default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated:
admin/admin

title: Collect System logs from Cassandra
description: Collecting System logs from Cassandra
- type: jolokia/metrics
vars:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can hide (show_user: false) few of these variables: path, username, password.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated
for path, username, password

@mtojek
Copy link
Contributor

mtojek commented Sep 30, 2021

/test

1 similar comment
@mtojek
Copy link
Contributor

mtojek commented Oct 4, 2021

/test

@mtojek
Copy link
Contributor

mtojek commented Oct 5, 2021

/test

@kush-elastic kush-elastic marked this pull request as ready for review October 5, 2021 12:23
@mtojek
Copy link
Contributor

mtojek commented Oct 5, 2021

/test

@mtojek mtojek self-requested a review October 5, 2021 12:56
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I don't see anything blocking here. You're good to go! Well done.

BTW I left a comment about service variants, but it isn't blocking. You can do this later on while maintaining the integration.

ports:
- 8778
cassandra-node-1:
image: cassandra:3.11.11
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm... you have service variants, but 3.11.11 here. Is there an option to test with other versions?

@@ -0,0 +1,4 @@
variants:
v3.11.11:
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but could you define it here to see if the integration works?

It can be done once this PR is merged (non-blocking).

@mtojek mtojek merged commit b5eaf5d into elastic:master Oct 6, 2021
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants