document implementation decisions #13729

iliakur · 2023-01-18T19:22:59Z

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 · 2023-01-18T19:23:16Z

Current dependencies on/for this PR:

master
- PR Support RabbitMQ Prometheus Metrics #13662
  - PR extend and refactor tests #13737
  - PR document implementation decisions #13729 👈
  - PR specify both detailed and aggregated endpoints in one instance #13727
  - PR validate check configuration values #13728
  - PR add more metrics to aggregated endpoint #13684
  - PR support for specifying just per-object endpoint #13692
  - PR support for specifying just detailed endpoint #13691
  - PR Only select aggregated endpoint #13673

This comment was auto-generated by Graphite.

iliakur · 2023-01-18T19:23:16Z

Current dependencies on/for this PR:

master
- PR Support RabbitMQ Prometheus Metrics #13662
  - PR Only select aggregated endpoint #13673
    - PR add more metrics to aggregated endpoint #13684
      - PR support for specifying just detailed endpoint #13691
        
        PR support for specifying just per-object endpoint #13692
        
        PR specify both detailed and aggregated endpoints in one instance #13727
        
        PR document implementation decisions #13729 👈
        
        PR validate check configuration values #13728

This comment was auto-generated by Graphite.

codecov · 2023-01-18T21:01:36Z

Codecov Report

❗ No coverage uploaded for pull request base (ik/AI-2736/rabbitmq-openmetrics@480a942). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 789d456 differs from pull request most recent head cc935e8. Consider uploading reports for the commit cc935e8 to get more accurate results

Flag	Coverage Δ
rabbitmq	`95.09% <0.00%> (?)`

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

fanny-jiang

LGTM

iliakur · 2023-01-19T17:36:23Z

Merging this since there I haven't found any more decisions to document right now and will do those in a separate PR or as part of the main one.

* Setting the stage (#13437) ### What does this PR do?  The Openmetrics-related changes are minimal: just a basic class and a test that mostly makes sure that this class can be imported and run. The rest is scaffolding: I needed new versions of rmq to test out my changes, the old tests needed adjusting to pass -> one config field had to be made optional to make sure my tests run -> etc... ### 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)](https://github.com/DataDog/integrations-core/blob/master/CONTRIBUTING.md#pull-request-title) - [ ] 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. * Only select aggregated endpoint (#13673) * only aggregated endpoint * fix erlang.vm.allocators assertion * copy suggestions Co-authored-by: Austin Lai <[email protected]> * Update rabbitmq/assets/configuration/spec.yaml Co-authored-by: Austin Lai <[email protected]> * Update models and example config to reflect current spec * Don't test rabbitmq 3.8 (openmetrics) with python2 * fix PY2 failure * consistent python env names; better setup to skip openmetrics tests * Add openmetrics to E2E environment (#13695) * add openmetrics e2e * re-add newline to compose file * revert hatch.toml changes Co-authored-by: Austin Lai <[email protected]> Co-authored-by: Fanny Jiang <[email protected]> * add more metrics to aggregated endpoint (#13684) * add more metrics to aggregated endpoint * separate out summary metrics * Refactor aggregated endpoint test (#13730) * refactor aggregated endpoints test * add missing metrics * fix dd metric names Co-authored-by: Fanny Jiang <[email protected]> * support for specifying just detailed endpoint (#13691) * support for specifying just detailed endpoint * Update rabbitmq/datadog_checks/rabbitmq/data/conf.yaml.example Co-authored-by: Bryce Eadie <[email protected]> * Update rabbitmq/datadog_checks/rabbitmq/data/conf.yaml.example Co-authored-by: Bryce Eadie <[email protected]> * sync config * apply and sync all the suggestions Co-authored-by: Bryce Eadie <[email protected]> * support for specifying just per-object endpoint (#13692) * support for specifying just per-object endpoint * Style fix: remove f-string without interpolation * specify both detailed and aggregated endpoints in one instance (#13727) * validate check configuration values (#13728) * validate check configuration values * several fixes: - remove unnecessary validation logic - fix url in one of the tests, - fix style - aggregated endpoint is enabled by default * extend and refactor tests (#13737) * extend and refactor tests * add comment explaining assertion * document implementation decisions (#13729) * fix license headers * Add openmetrics E2E test (#13744) * enable openmetrics e2e test * assert metrics * small docs fixes (#13746) * add license header to test file * Define multiple instances in config spec (#13742) * wip * resync config * document example detailed query endpoint * fix e2e test * remove example code from validators * resync config spec * add missing fields to metadata.csv, fix metric name (#13745) * 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]> * readme draft 1 * readme, migration guide * split integration and e2e tests * Apply suggestions from code review Co-authored-by: Fanny Jiang <[email protected]> * fix license header * move section * remove "we" pronoun * Update rabbitmq/README.md Co-authored-by: Esther Kim <[email protected]> * add migration guide section * fix e2e test Co-authored-by: Austin Lai <[email protected]> Co-authored-by: Fanny Jiang <[email protected]> Co-authored-by: Bryce Eadie <[email protected]> Co-authored-by: Esther Kim <[email protected]>

ghost added the integration/rabbitmq label Jan 18, 2023

This was referenced Jan 18, 2023

specify both detailed and aggregated endpoints in one instance #13727

Merged

validate check configuration values #13728

Merged

iliakur added the changelog/no-changelog label Jan 18, 2023

fanny-jiang approved these changes Jan 18, 2023

View reviewed changes

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from ae48195 to 76e853a Compare January 18, 2023 21:55

iliakur force-pushed the 01-18-document_implementation_decisions branch from 789d456 to e9e791f Compare January 18, 2023 21:55

iliakur mentioned this pull request Jan 19, 2023

extend and refactor tests #13737

Merged

5 tasks

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from 76e853a to bc7d6ed Compare January 19, 2023 17:02

iliakur force-pushed the 01-18-document_implementation_decisions branch from e9e791f to 598e8b7 Compare January 19, 2023 17:02

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from bc7d6ed to 4c1cade Compare January 19, 2023 17:16

iliakur force-pushed the 01-18-document_implementation_decisions branch from 598e8b7 to 6cb3f41 Compare January 19, 2023 17:16

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from 4c1cade to 32b2646 Compare January 19, 2023 17:22

iliakur force-pushed the 01-18-document_implementation_decisions branch from 6cb3f41 to baef7f9 Compare January 19, 2023 17:22

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from 32b2646 to 7e09ecb Compare January 19, 2023 17:23

iliakur force-pushed the 01-18-document_implementation_decisions branch from baef7f9 to 1b2c73e Compare January 19, 2023 17:24

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from 7e09ecb to e5ea938 Compare January 19, 2023 17:28

iliakur force-pushed the 01-18-document_implementation_decisions branch from 1b2c73e to 57c869f Compare January 19, 2023 17:28

iliakur force-pushed the 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance branch from e5ea938 to 9226746 Compare January 19, 2023 17:30

iliakur force-pushed the 01-18-document_implementation_decisions branch from 57c869f to b400c9b Compare January 19, 2023 17:31

Base automatically changed from 01-18-specify_both_detailed_and_aggregated_endpoints_in_one_instance to ik/AI-2736/rabbitmq-openmetrics January 19, 2023 17:31

iliakur force-pushed the 01-18-document_implementation_decisions branch from b400c9b to 59af001 Compare January 19, 2023 17:32

iliakur force-pushed the 01-18-document_implementation_decisions branch from 59af001 to 79777e1 Compare January 19, 2023 17:33

document implementation decisions

cc935e8

iliakur force-pushed the 01-18-document_implementation_decisions branch from 79777e1 to cc935e8 Compare January 19, 2023 17:35

iliakur marked this pull request as ready for review January 19, 2023 17:35

iliakur requested a review from a team as a code owner January 19, 2023 17:35

iliakur merged commit 0bb109e into ik/AI-2736/rabbitmq-openmetrics Jan 19, 2023

iliakur deleted the 01-18-document_implementation_decisions branch January 19, 2023 17:36

iliakur added a commit that referenced this pull request Jan 19, 2023

document implementation decisions (#13729)

bcca598

iliakur added a commit that referenced this pull request Jan 20, 2023

document implementation decisions (#13729)

6504e2e

iliakur added a commit that referenced this pull request Jan 20, 2023

document implementation decisions (#13729)

a32e113

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

document implementation decisions #13729

document implementation decisions #13729

iliakur commented Jan 18, 2023

iliakur commented Jan 18, 2023 •

edited

Loading

iliakur commented Jan 18, 2023

codecov bot commented Jan 18, 2023 •

edited

Loading

fanny-jiang left a comment

iliakur commented Jan 19, 2023

document implementation decisions #13729

document implementation decisions #13729

Conversation

iliakur commented Jan 18, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

iliakur commented Jan 18, 2023 • edited Loading

iliakur commented Jan 18, 2023

codecov bot commented Jan 18, 2023 • edited Loading

Codecov Report

fanny-jiang left a comment

Choose a reason for hiding this comment

iliakur commented Jan 19, 2023

iliakur commented Jan 18, 2023 •

edited

Loading

codecov bot commented Jan 18, 2023 •

edited

Loading