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

Support RabbitMQ Prometheus Metrics #13662

Merged
merged 31 commits into from
Jan 20, 2023
Merged

Support RabbitMQ Prometheus Metrics #13662

merged 31 commits into from
Jan 20, 2023

Conversation

iliakur
Copy link
Contributor

@iliakur iliakur commented Jan 10, 2023 •
edited by yzhan289
Loading

What does this PR do?

This PR contains all the changes for enabling openmetrics/prometheus support in rabbitmq integration.

For visibility, some things to check when reviewing:

  • we test that rabbitmq_api_url is present in case the legacy check is chosen
  • ...

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@iliakur iliakur requested review from a team as code owners January 10, 2023 09:11
@iliakur iliakur marked this pull request as draft January 10, 2023 09:49
@iliakur iliakur mentioned this pull request Jan 11, 2023
Merged
5 tasks
@iliakur Graphite App
Copy link
Contributor Author

iliakur commented Jan 11, 2023 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@iliakur iliakur force-pushed the ik/AI-2736/rabbitmq-openmetrics branch from e4e0d78 to 0fc6877 Compare January 12, 2023 17:12
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

This was referenced Jan 12, 2023
Merged
Merged
Merged
Merged
Merged
@iliakur iliakur force-pushed the ik/AI-2736/rabbitmq-openmetrics branch from 0fc6877 to fea2aea Compare January 18, 2023 19:21
@iliakur iliakur mentioned this pull request Jan 18, 2023
Merged
5 tasks
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@iliakur iliakur mentioned this pull request Jan 19, 2023
Merged
5 tasks
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

7 similar comments
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@iliakur iliakur force-pushed the ik/AI-2736/rabbitmq-openmetrics branch from 0bb109e to 7707917 Compare January 19, 2023 17:55
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@codecov
Copy link

codecov bot commented Jan 19, 2023 •
edited
Loading

Codecov Report

Merging #13662 (a26ea7b) into master (071d153) will increase coverage by 0.01%.
The diff coverage is 99.04%.

Flag Coverage Δ
rabbitmq 95.53% <99.04%> (+1.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

iliakur and others added 5 commits January 20, 2023 17:41
@iliakur @fanny-jiang
add missing fields to metadata.csv, fix metric name (#13745)
de5a145 
* add missing fields to metadata.csv, fix metric name

* add special detailed metrics, fix validation errors

* Apply suggestions from code review

Co-authored-by: Fanny Jiang <[email protected]>

* remove last "total" suffixes

* add missing unit names

Co-authored-by: Fanny Jiang <[email protected]>
@iliakur
readme draft 1
75d357f
@iliakur
readme, migration guide
b41a1fd
@fanny-jiang
split integration and e2e tests
6eb3f3c
@fanny-jiang
Merge branch 'fanny/split-int-e2e-tests' into ik/AI-2736/rabbitmq-ope…
9c6f73c 
…nmetrics
fanny-jiang
fanny-jiang reviewed Jan 20, 2023
View reviewed changes
rabbitmq/README.md Outdated Show resolved Hide resolved
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

fanny-jiang
fanny-jiang reviewed Jan 20, 2023
View reviewed changes
rabbitmq/README.md Outdated Show resolved Hide resolved
fanny-jiang
fanny-jiang reviewed Jan 20, 2023
View reviewed changes
rabbitmq/README.md Outdated Show resolved Hide resolved
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

estherk15
estherk15 previously approved these changes Jan 20, 2023
View reviewed changes
rabbitmq/README.md Outdated Show resolved Hide resolved
@iliakur @estherk15
Update rabbitmq/README.md
da2ed6a 
Co-authored-by: Esther Kim <[email protected]>
steveny91
steveny91 reviewed Jan 20, 2023
View reviewed changes
rabbitmq/assets/configuration/spec.yaml
value:
type: string
example: 'http://localhost:15692'
- name: include_aggregated_endpoint
Copy link
Contributor

@steveny91 steveny91 Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What happens if this is false? Do we scrape anything still? If so where?

Copy link
Contributor

@fanny-jiang fanny-jiang Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If it's false and no other endpoints are configured, then nothing gets collected. It's mostly there to disable aggregated collection if the detailed endpoint collection is enabled because the detailed endpoint includes metrics from the aggregated endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interesting case. I would actually be tempted to raise an error if no endpoints are specified, since chugging along silently here doesn't make sense.

@fanny-jiang what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, I think it would be good raise an error as part of the validations. I'll add this to the QA.

Copy link
Contributor

Choose a reason for hiding this comment

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

#13793

@fanny-jiang fanny-jiang merged commit 7f9a405 into master Jan 20, 2023
@fanny-jiang fanny-jiang deleted the ik/AI-2736/rabbitmq-openmetrics branch January 20, 2023 19:59
@yzhan289 yzhan289 mentioned this pull request Jan 24, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yzhan289 yzhan289 yzhan289 left review comments

@steveny91 steveny91 steveny91 approved these changes

@fanny-jiang fanny-jiang fanny-jiang approved these changes

@estherk15 estherk15 estherk15 approved these changes

Assignees
No one assigned
Labels
documentation integration/rabbitmq
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@iliakur @steveny91 @fanny-jiang @yzhan289 @estherk15