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 new floorplan with new queries for weekly data collection #1955

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add new floorplan with new queries for weekly data collection #1955

wants to merge 11 commits into from
+33 −1

Conversation

thearifismail
Copy link
Contributor

rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED

Overview

This PR is being created to address RHINENG-12228.
@vkrizan, here is the new PR for collecting weekly numbers for new and updated hosts and groups. Since these metrics will use a new S3 bucket, these new queries may have their own floorplan. So I have add a new one named "host-inventory-success-metrics".

For getting a new S3 bucket, I looked into app-interface but did not find much except hccm using s3-buckets and it did not show how to create or deploy an S3 using app-interface. This means I need your help. For now, I am using the other floorplan's parameter as space fillers.

PR Checklist

  • Keep PR title short, ideally under 72 characters
  • Descriptive comments provided in complex code blocks
  • Include raw query examples in the PR description, if adding/modifying SQL query
  • Tests: validate optimal/expected output
  • Tests: validate exceptions and failure scenarios
  • Tests: edge cases
  • Recovers or fails gracefully during potential resource outages (e.g. DB, Kafka)
  • Uses type hinting, if convenient
  • Documentation, if this PR changes the way other services interact with host inventory
  • Links to related PRs

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

@thearifismail
Add new floorplan with new queries for weekly data collection
a1c1585 
rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@thearifismail thearifismail requested a review from a team as a code owner September 24, 2024 15:53
vkrizan
vkrizan reviewed Sep 24, 2024
View reviewed changes
Copy link
Contributor

@vkrizan vkrizan left a comment

Choose a reason for hiding this comment

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

@thearifismail This is much much better. I've left some final comments.

Thank you.

deploy/clowdapp.yml Outdated
- apiVersion: metrics.console.redhat.com/v1alpha1
kind: FloorPlan
metadata:
name: host-inventory-success-metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please shorten this? This name is used to build names of child resources.

I'd suggest host-inventory-hms, to keep it consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as suggested.

deploy/clowdapp.yml Outdated
database:
secretName: ${FLOORIST_DB_SECRET_NAME}
objectStore:
secretName: ${FLOORIST_BUCKET_SECRET_NAME}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please define a separate template variable. Suggested name FLOORIST_HMS_BUCKET_SECRET_NAME.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as suggested.

deploy/clowdapp.yml Outdated
suspend: ${{FLOORIST_SUSPEND}}
logLevel: ${FLOORIST_LOGLEVEL}
queries:
- prefix: insights/inventory/hosts_success_metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

You can shorten these as well insights/inventory/hosts would do. Using the same name/prefix is okay, as this would be in a different S3 bucket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as suggested.

deploy/clowdapp.yml Outdated
query: >-
SELECT
"id",
"account" AS "account_number",
Copy link
Contributor

Choose a reason for hiding this comment

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

As account can be NULL, and the exporting tool has issues to figure out the type if it occurs on the first record, I recommend to convert this to text account::TEXT or use COALESCE(account, 0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about the exact syntax, but this is what I changed it:

COALESCE("account", 0) AS "account_number",

@thearifismail
responsed to review comments
2b479cf 
rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@thearifismail
Copy link
Contributor Author

@vkrizan, can you help with S3 bucket setup? I checked #sd-app-sre channel for S3 and found how to install aws_s3 extension but we don't have direct access to RDS. Since a floorplan is already setup in Host-Inventory, I am assuming that the aws_s3 extension is already installed. Next question is how do I create an S3 bucket? I thought this whole S3 setup would be handled by app-interface but where to start has been alluding me.

vkrizan
vkrizan approved these changes Sep 26, 2024
View reviewed changes
Copy link
Contributor

@vkrizan vkrizan left a comment

Choose a reason for hiding this comment

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

Besides the small thing, this looks good!

Before merging, ensure that the secret is properly mapped in the deployments.

deploy/clowdapp.yml Outdated
"created_on" AS "created_at",
"modified_on" AS "updated_at",
FROM "hosts"
- prefix: insights/inventory/groups_success_metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

This also can be shorter: insights/inventory/groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!!!

@vkrizan
Copy link
Contributor

vkrizan commented Sep 26, 2024

@thearifismail The bucket exists. I've messaged you directly more information with examples.

dependabot bot and others added 5 commits September 26, 2024 08:03
@dependabot @thearifismail
build(deps): bump certifi from 2024.7.4 to 2024.8.30 (RedHatInsights#…
f0bceb6 
…1956)

Bumps [certifi](https://github.com/certifi/python-certifi) from 2024.7.4 to 2024.8.30.
- [Commits](certifi/python-certifi@2024.07.04...2024.08.30)

---
updated-dependencies:
- dependency-name: certifi
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @thearifismail
build(deps): bump prometheus-client from 0.20.0 to 0.21.0 (RedHatInsi…
ce83053 
…ghts#1952)

Bumps [prometheus-client](https://github.com/prometheus/client_python) from 0.20.0 to 0.21.0.
- [Release notes](https://github.com/prometheus/client_python/releases)
- [Commits](prometheus/client_python@v0.20.0...v0.21.0)

---
updated-dependencies:
- dependency-name: prometheus-client
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@fstavela @thearifismail
Update all-tests pipeline (RedHatInsights#1957)
8163f75 
rh-pre-commit.version: 2.3.1
rh-pre-commit.check-secrets: ENABLED
@thearifismail
Fix the groups query name
9b7eda8 
rh-pre-commit.version: 2.2.0
rh-pre-commit.check-secrets: ENABLED
@thearifismail
Merge branch 'master' into hbi_success_metrics
5b44956
FabriciaDinizRH
FabriciaDinizRH approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@FabriciaDinizRH FabriciaDinizRH left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kruai kruai kruai approved these changes

@vkrizan vkrizan vkrizan approved these changes

@FabriciaDinizRH FabriciaDinizRH FabriciaDinizRH approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thearifismail @vkrizan @kruai @FabriciaDinizRH @fstavela