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

Gauges for global publishers & consumers metrics #3136

Merged
merged 3 commits into from
Jun 29, 2021
Merged

Conversation

dcorbacho
Copy link
Contributor

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@dcorbacho dcorbacho requested a review from gerhard June 23, 2021 10:27
Copy link
Contributor

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

This looks good! These items are missing before we can merge:

  • modify deps/rabbit/queue_type_SUITE > smoke to check for these new metrics
  • modify deps/rabbitmq_prometheus/rabbit_prometheus_http_SUITE.erl > global_metrics_present_test to check for these new metrics
  • add metric to deps/rabbitmq_prometheus/metrics.md under GLOBAL COUNTERS

While reviewing this, I realised that we should remove the concept of publishers when using the AMQP 0.9.1 protocol as we can't really tell how many publishers there are. Channels can both consume and publish (bad idea, but some users still do this), and channels which are not subscribed to any queues doesn't necessarily mean that they are publishing channels (a.k.a. publishers).

Almost there 💪

@gerhard
Copy link
Contributor

gerhard commented Jun 24, 2021

@pjk25 is there anything that we need to do for the tests to pass? I wouldn't like to merge this before all checks are green, which includes OTP 23 & 24 tests.

@HoloRin
Copy link
Contributor

HoloRin commented Jun 24, 2021

@gerhard I believe you need this commit e666a1f though it would probably make more sense to rebase or merge master -> this branch so that you pick up additional flaky test annotations that I have pushed recently.

@dcorbacho dcorbacho requested a review from gerhard June 25, 2021 14:19
@gerhard
Copy link
Contributor

gerhard commented Jun 25, 2021

Did a quick deploy into the 3.9 LRE, looking good:

image

The tests are failing which is a blocker, and I'm seeing this locally too:

rabbit_stream_queue_SUITE > cluster_size_2_parallel > update_retention_policy
    #1. {error,
            {{assert,
                 [{module,quorum_queue_utils},
                  {line,89},
                  {expression,"binary_to_integer ( Got ) >= Msgs"},
                  {expected,true},
                  {value,false}]},
             [{quorum_queue_utils,wait_for_min_messages,4,
                  [{file,"test/quorum_queue_utils.erl"},{line,89}]},
              {rabbit_stream_queue_SUITE,update_retention_policy,1,
                  [{file,"test/rabbit_stream_queue_SUITE.erl"},{line,1813}]},
              {test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
              {test_server,run_test_case_eval1,6,
                  [{file,"test_server.erl"},{line,1292}]},
              {test_server,run_test_case_eval,9,
                  [{file,"test_server.erl"},{line,1224}]}]}}

There could be other failures, I only ran this test suite locally, but we should definitely work towards getting all above checks green. Tests passing is the only blocker at this point, everything else is done and ready to merge.

I will kick off the tests, but then stopping for the week 🌄

@gerhard gerhard force-pushed the global-gauges branch 2 times, most recently from 7fdbae5 to ddea2ae Compare June 28, 2021 09:10
@gerhard gerhard merged commit 3078d05 into master Jun 29, 2021
@gerhard gerhard deleted the global-gauges branch June 29, 2021 08:21
gerhard added a commit that referenced this pull request Jun 29, 2021
Gauges for global publishers & consumers metrics

(cherry picked from commit 3078d05)
@gerhard
Copy link
Contributor

gerhard commented Jun 29, 2021

Backported to v3.9.x

The v3.8.x back-port will need to wait on #3127 being backported first. Adding it to that to-do list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants