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

feat(RHIDP-872): Enable RHDH nodejs metrics #75

Conversation

yogananth-subramanian
Copy link
Collaborator

Enable collecting RHDH nodejs prometheus metrics.
The environement variable RHDH_METRIC is set to true to enable collecting the metrics.

@openshift-ci openshift-ci bot requested review from jhutar and pmacik September 5, 2024 06:22
@openshift-ci openshift-ci bot added the approved label Sep 5, 2024
monitoring_step: 15
{%- endmacro %}

{% for query in [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here it might be easier to read just a multiple calls to that macro instead of a loop with these ifs. And maybe learn rhdh_nodejs_lst to accept value_list instead of just value:

rhdh_nodejs_lst('catalog_processors_duration_seconds_sum', 'result', ['ok', 'failed'])
rhdh_nodejs_lst('catalog_processors_duration_seconds_count', 'result', ['ok', 'failed'])
rhdh_nodejs_lst('catalog_processing_duration_seconds_sum', 'result', ['unchanged'])
rhdh_nodejs_lst('catalog_processing_duration_seconds_count', 'result', ['unchanged'])
rhdh_nodejs_lst('nodejs_gc_duration_seconds_sum', 'kind', ['minor', 'major', 'incremental'])
rhdh_nodejs_lst('nodejs_gc_duration_seconds_count', 'kind', ['minor', 'major', 'incremental'])
rhdh_nodejs_lst('catalog_entities_count', 'kind', ['location', 'user', 'group'])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about it more, are these metrics important for us? Or do we want to just collect averages?

catalog_processors_duration_seconds_sum{result="ok"} / catalog_processors_duration_seconds_count{result="ok"}

And maybe just number of failed ones?

rate(catalog_processors_duration_seconds_count{result="failed"}[5m])

I mean - instead of collecting all/lots of the individual metrics that are there, just collect these we actually care about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hello @jhutar , thanks for the comments, in the latest commit I have rewritten metric collection in more readable way, as you have suggested and also added metrics for gathering rates and averages instead of just raw data.

@jhutar jhutar changed the title Enable RHDH nodejs metrics feat(RHIDP-872): Enable RHDH nodejs metrics Sep 5, 2024
@yogananth-subramanian yogananth-subramanian force-pushed the rhdh-metrics branch 3 times, most recently from ec67382 to 89ccd7e Compare September 10, 2024 08:24
Enable collecting RHDH nodejs prometheus metrics.
The environement variable RHDH_METRIC is set to true to enable collecting
the metrics.
Copy link
Collaborator

@jhutar jhutar left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Sep 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhutar, yogananth-subramanian

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:
  • OWNERS [jhutar,yogananth-subramanian]

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

@jhutar
Copy link
Collaborator

jhutar commented Sep 12, 2024

Thank you!

@openshift-merge-bot openshift-merge-bot bot merged commit d630f48 into redhat-performance:main Sep 12, 2024
3 checks passed
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.

2 participants