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

Make cockroachDB GA #3676

Merged
merged 4 commits into from
Sep 20, 2022
Merged

Conversation

ritalwar
Copy link
Contributor

@ritalwar ritalwar commented Jul 11, 2022

  • Enhancement

What does this PR do?

This PR make CockroachDB release GA

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

Run the cockroachDB instance.
Add the integration and metrics should start flowing through Kibana.

Related issues

Screenshots

@elasticmachine
Copy link

elasticmachine commented Jul 11, 2022

💚 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: 2022-09-13T10:57:51.713+0000

  • Duration: 14 min 43 sec

Test stats 🧪

Test Results
Failed 0
Passed 4
Skipped 0
Total 4

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jul 11, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚 2.703
Classes 100.0% (0/0) 💚 2.703
Methods 75.0% (3/4) 👎 -14.73
Lines 100.0% (0/0) 💚 8.611
Conditionals 100.0% (0/0) 💚

- {{this}}
{{/each}}
- raft_process_handleready_latency
- raft_scheduler_latency
Copy link
Contributor

Choose a reason for hiding this comment

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

For now are we excluding all the histogram metrics ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, once dynamic template mapping issue is resolved , will remove these from here.

@ishleenk17
Copy link
Contributor

ecs.yml and base-fields.yml is missing. Please add those

@ishleenk17
Copy link
Contributor

I think there is some issue with the basic structuring of the integration.
I don't see README under _dev/build.
That is used to generate the main rEADME. How are we getting the main README currently ?

@ritalwar
Copy link
Contributor Author

I think there is some issue with the basic structuring of the integration. I don't see README under _dev/build. That is used to generate the main rEADME. How are we getting the main README currently ?

It is under _dev/build/docs, Please see : https://github.com/ritalwar/integrations/blob/migrate_cockroachdb_ga_3550/packages/cockroachdb/_dev/build/docs/README.md and this is being used to generate main README.

@ishleenk17
Copy link
Contributor

I think there is some issue with the basic structuring of the integration. I don't see README under _dev/build. That is used to generate the main rEADME. How are we getting the main README currently ?

It is under _dev/build/docs, Please see : https://github.com/ritalwar/integrations/blob/migrate_cockroachdb_ga_3550/packages/cockroachdb/_dev/build/docs/README.md and this is being used to generate main README.

My bad, I thought it to be a new integration. Hence was expecting all the files.

@ritalwar
Copy link
Contributor Author

ecs.yml and base-fields.yml is missing. Please add those

It is already there, https://github.com/ritalwar/integrations/tree/migrate_cockroachdb_ga_3550/packages/cockroachdb/data_stream/status/fields

@andrewkroh andrewkroh added Integration:cockroachdb CockroachDB Metrics Team:Service-Integrations Label for the Service Integrations team labels Aug 15, 2022
@ritalwar ritalwar marked this pull request as ready for review September 6, 2022 10:11
@ritalwar ritalwar requested a review from a team as a code owner September 6, 2022 10:11
@ishleenk17
Copy link
Contributor

  1. Does this work if no username/pwd is provided to the db ? As I don't see any default value for it.

  2. We are doing a * mapping for the fields. As you informed there are around 950 metrics that get exported.
    Are we making all of these available for user ? The sample json file has a subset of these.

@ishleenk17
Copy link
Contributor

Have we tested the integrations with bearer token and ssl.certificate_authorities input ?
Since that is available as a user input.

@ritalwar ritalwar mentioned this pull request Sep 8, 2022
8 tasks
@ritalwar
Copy link
Contributor Author

  1. Does this work if no username/pwd is provided to the db ? As I don't see any default value for it.

Yes, it works without username and pwd also and if DB is secured by username/pwd then we can provide this. These are optional fields hence no default value.

  1. We are doing a * mapping for the fields. As you informed there are around 950 metrics that get exported.
    Are we making all of these available for user ? The sample json file has a subset of these.

Yes, all these are available for user.

@ishleenk17
Copy link
Contributor

Yes, all these are available for user.

The sample json is quite a smaller set. Are those cumulative values of those counters ?

@ritalwar
Copy link
Contributor Author

ritalwar commented Sep 13, 2022

Yes, all these are available for user.

The sample json is quite a smaller set. Are those cumulative values of those counters ?

No, I think that's because of cockroachdb version getting used in system test, with this version these many fields were generated and I reported around 900 fields with latest version of cockroachDb i.e v22.1.6.
I tried with same version of cockroachDb in system test also and now the field count in sample_event.json is almost same, shall I update this version and commit this sample_event.json file?

@ishleenk17
Copy link
Contributor

Yes, all these are available for user.

The sample json is quite a smaller set. Are those cumulative values of those counters ?

No, I think that's because of cockroachdb version getting used in system test, with this version these many fields were generated and I reported around 900 fields with latest version of cockroachDb i.e v22.1.6. I tried with same version of cockroachDb in system test also and now the field count in sample_event.json is almost same, shall I update this version and commit this sample_event.json file?

I think verifying this should be sufficient. Having those many fields listed on the UI is not required.

@ritalwar
Copy link
Contributor Author

Have we tested the integrations with bearer token and ssl.certificate_authorities input ? Since that is available as a user input.

Yes, Testing has been done with both the options now.

Copy link
Contributor

@ishleenk17 ishleenk17 left a comment

Choose a reason for hiding this comment

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

Looks good!

@ritalwar ritalwar merged commit bc887ef into elastic:main Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration:cockroachdb CockroachDB Metrics Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CockroachDB GA Move cockroachdb module to GA
4 participants