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

Expose max offset lag of stream consumers via Prometheus - backport to 4.0.x #12814

Conversation

gomoripeti
Copy link
Contributor

Proposed Changes

This is #12765 manually backported to v4.0.x

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • 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)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

Further Comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc.

gomoripeti and others added 4 commits November 26, 2024 09:16
So that they can be used from multiple test suites.
Supports both per stream (detailed) and aggregated (metrics) values.
The application is not always recompiled which causes tests to fail
because they cannot call `serial_number:usort/1`.
@michaelklishin
Copy link
Member

@gomoripeti v4.0.x still uses Bazel for CI, so please run bazel run gazelle when you add, delete or rename modules.

@michaelklishin
Copy link
Member

michaelklishin commented Nov 26, 2024

@gomoripeti the failures in #12820 now (after bazel run gazelle) come down to
an {error,econnrefused}:

=== Ended at 2024-11-26 19:38:57
=== Location: [{stream_test_utils,connect,21},
              {rabbit_prometheus_http_SUITE,publish_via_stream_protocol,794},
              {rabbit_prometheus_http_SUITE,stream_pub_sub_metrics,716},
              {test_server,ts_tc,1793},
              {test_server,run_test_case_eval1,1302},
              {test_server,run_test_case_eval,1234}]
=== === Reason: no match of right hand side value {error,econnrefused}
  in function  stream_test_utils:connect/2 (stream_test_utils.erl, line 21)
  in call from rabbit_prometheus_http_SUITE:publish_via_stream_protocol/3 (rabbit_prometheus_http_SUITE.erl, line 794)
  in call from rabbit_prometheus_http_SUITE:stream_pub_sub_metrics/1 (rabbit_prometheus_http_SUITE.erl, line 716)
  in call from test_server:ts_tc/3 (test_server.erl, line 1793)
  in call from test_server:run_test_case_eval1/6 (test_server.erl, line 1302)
  in call from test_server:run_test_case_eval/9 (test_server.erl, line 1234)

So the stream plugin is not enabled, or not yet enabled, on the target node.

@michaelklishin
Copy link
Member

michaelklishin commented Nov 26, 2024

Another possible reason: the connecting assumes a certain port and it is not guaranteed to be stable. It can be retrieved from node config by protocol name, for example.

Update: no,

StreamPort = rabbit_ct_broker_helpers:get_node_config(Config, Node, tcp_port_stream),

is exactly what stream_test_utils:connect/2 does.

@michaelklishin
Copy link
Member

For Bazel, rabbitmq_stream wasn't enabled for the integration tests node. With 2846f39 the suite now passes for me locally.

@gomoripeti
Copy link
Contributor Author

sorry about the hustle, I did not realize that 4.0.x still uses bazel (and I don't have that set up locally)

@michaelklishin
Copy link
Member

michaelklishin commented Nov 26, 2024

CI still uses Bazel, you don't have to use it locally except for cases like this. Soon enough CI in v4.0.x will follow main.

@michaelklishin
Copy link
Member

If you choose to use Bazel locally at least for running tests at the end, use a different clone. Make and Bazel cannot co-exist in a single clone, dependencies under deps will confuse Bazel which expects a very detailed description of project structure.

@michaelklishin
Copy link
Member

#12820 is in. Thank you for contributing.

@michaelklishin
Copy link
Member

This change is included into 4.0.5-alpha.5d8a80db.

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

Successfully merging this pull request may close these issues.

3 participants