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

MGMT-11309: add prometheus expoter for postgresql #4216

Merged

Conversation

rccrdpccl
Copy link
Contributor

This PR adds prometheus exporter for assisted-installer.
It has been added to a separate template file, so that we can deploy it indipendently as it is not related directly with AI code, but only with its dependency.
This will allow us to have better insights on PG metrics. In this specific instance we were looking to count added events, and we're trying to achieve this with the autoincrement counter (to account for deletions).
It also exposes other pg metrics that can be useful as replication lag and such (default in helm template used to generate this manifest)

List all the issues related to this PR

  • New Feature
  • Enhancement
  • Bug fix
  • Tests
  • Documentation
  • CI/CD
  • monitoring

What environments does this code impact?

  • Automation (CI, tools, etc)
  • Cloud
  • Operator Managed Deployments
  • None

How was this code tested?

  • assisted-test-infra environment
  • dev-scripts environment
  • Reviewer's test appreciated
  • Waiting for CI to do a full test run
  • Manual (Elaborate on how it was tested)
  • No tests needed

Assignees

/cc @gamli75

Checklist

  • Title and description added to both, commit and PR.
  • Relevant issues have been associated (see CONTRIBUTING guide)
  • Reviewers have been listed
  • This change does not require a documentation update (docstring, docs, README, etc)
  • Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

  • Are the title and description (in both PR and commit) meaningful and clear?
  • Is there a bug required (and linked) for this change?
  • Should this PR be backported?

@openshift-ci openshift-ci bot requested a review from gamli75 August 1, 2022 10:55
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 1, 2022
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #4216 (198fd05) into master (ed0ea92) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4216   +/-   ##
=======================================
  Coverage   65.56%   65.56%           
=======================================
  Files         189      189           
  Lines       26484    26484           
=======================================
  Hits        17365    17365           
  Misses       7515     7515           
  Partials     1604     1604           
Impacted Files Coverage Δ
internal/cluster/validations/validations.go 44.02% <0.00%> (-0.75%) ⬇️
internal/oc/release.go 71.79% <0.00%> (+1.28%) ⬆️

@openshift-ci
Copy link

openshift-ci bot commented Aug 1, 2022

@rccrdpccl: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rccrdpccl
Copy link
Contributor Author

/cc @osherdp

@openshift-ci openshift-ci bot requested a review from osherdp August 1, 2022 12:12
@osherdp
Copy link
Contributor

osherdp commented Aug 1, 2022

Is it possible having it in a different repository? like assisted-events-scraper

@rccrdpccl
Copy link
Contributor Author

Sure, I can move it somewhere else, but I feel like does not belong to scraper either, as postgresql instance belongs to assisted-service.
I could create another dedicated repository for it, WDYT?

@osherdp
Copy link
Contributor

osherdp commented Aug 1, 2022

Sure, I can move it somewhere else, but I feel like does not belong to scraper either, as postgresql instance belongs to assisted-service. I could create another dedicated repository for it, WDYT?

If it's not too much work. Because I think assisted-service jobs will eventually slow us down by validating all those changes with unit-tests and those other jobs

Another option that might be good enough for now is to make those jobs to not trigger on changes to those template files. Is it possible changing configuration for unit-test and verify-generated-code jobs?

@rccrdpccl
Copy link
Contributor Author

If it's not too much work. Because I think assisted-service jobs will eventually slow us down by validating all those changes with unit-tests and those other jobs

Another option that might be good enough for now is to make those jobs to not trigger on changes to those template files. Is it possible changing configuration for unit-test and verify-generated-code jobs?

I see. I am interested in doing this the best way possible, as it won't take too much either way (new repo already created, though it could be deleted if necessary - https://github.com/openshift-assisted/postgres-monitoring/pull/1). Which way would you think it's best?

I would think it makes sense to have it in the assisted-service repo just because the DB belongs to it, however I might be missing some context that would make me change opinion.

In any case I will change the way the jobs unit-test and verify-generated-code are triggered so they won't run on openshift templates changes.

@rccrdpccl
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2022
@osherdp
Copy link
Contributor

osherdp commented Aug 1, 2022

If it's not too much work. Because I think assisted-service jobs will eventually slow us down by validating all those changes with unit-tests and those other jobs
Another option that might be good enough for now is to make those jobs to not trigger on changes to those template files. Is it possible changing configuration for unit-test and verify-generated-code jobs?

I see. I am interested in doing this the best way possible, as it won't take too much either way (new repo already created, though it could be deleted if necessary - openshift-assisted/postgres-monitoring#1). Which way would you think it's best?

I would think it makes sense to have it in the assisted-service repo just because the DB belongs to it, however I might be missing some context that would make me change opinion.

In any case I will change the way the jobs unit-test and verify-generated-code are triggered so they won't run on openshift templates changes.

maybe you're right and it should reside in assisted-service repo
I don't have a very strong opinion on that

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2022
@openshift-ci
Copy link

openshift-ci bot commented Aug 1, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: osherdp, rccrdpccl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rccrdpccl
Copy link
Contributor Author

related: openshift/release#30966

@rccrdpccl
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2022
@openshift-merge-robot openshift-merge-robot merged commit 94ac355 into openshift:master Aug 1, 2022
danielerez pushed a commit to danielerez/assisted-service that referenced this pull request Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants