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

ARTEMIS-4498 - Expose internal queues for management and observability #4670

Closed
wants to merge 1 commit into from

Conversation

AntonRoskvist
Copy link
Contributor

This was enabled by default in previous versions of the broker and where quite good for troubleshooting and observability purposes. Adding it here as a configurable option defaulting to the current behavior

@clebertsuconic
Copy link
Contributor

I wonder if we shouldn't always show them? make a positive change here?

maybe add a field to say they are internal queues?

@clebertsuconic
Copy link
Contributor

a field on the web console I mean?

also we need to make sure the queues from broker connection would show as internal queues as well.

@AntonRoskvist
Copy link
Contributor Author

@clebertsuconic
That sounds good to me, though I am not sure what you mean about the field in the web console... there already exists a value for "internal" on the address level, would that be what you are referring to? Or you want it added to the queue level as well?

@clebertsuconic
Copy link
Contributor

I mean to be added.

@AntonRoskvist
Copy link
Contributor Author

@clebertsuconic
I have made some changes that I hope aligns with what you requested. If you agree that this is looking good I'll squash the commits.

@AntonRoskvist
Copy link
Contributor Author

I went ahead and squashed the commits as the changes overview looked really messy before...

@clebertsuconic
Copy link
Contributor

I think the management view, where you list the queues should have a field isInternal visible.
Columns

@clebertsuconic
Copy link
Contributor

@AntonRoskvist I have recently added a field into Consumer's with minimal knowledge of the admin console's source code. on commit 11f76bc.

let me know if you can do that change?

@clebertsuconic
Copy link
Contributor

clebertsuconic commented Nov 29, 2023

there are a few failures also:

  • o.a.a.a.t.i.m.ActiveMQServerControlTest.testListQueues[legacyCreateQueue=true]
  • o.a.a.a.t.i.m.ActiveMQServerControlTest.testListQueuesNumericFilter[legacyCreateQueue=true]
  • o.a.a.a.t.i.m.ActiveMQServerControlTest.testListQueues[legacyCreateQueue=false]
  • o.a.a.a.t.i.m.ActiveMQServerControlTest.testListQueuesNumericFilter[legacyCreateQueue=false]
  • o.a.a.a.t.i.s.QueueConfigPersistenceTest.testInternalQueue

@AntonRoskvist
Copy link
Contributor Author

@clebertsuconic

Oh, I thought you meant to add internal as an attribute on the queue. I'm keeping that first one and also add it as a column field.
I also fixed the tests you noticed where failing.

Is this looking alright?

@jbertram
Copy link
Contributor

jbertram commented Dec 4, 2023

I went a different direction on this. You can see it on #4702. See the updated description on ARTEMIS-4498 for additional details.

@AntonRoskvist
Copy link
Contributor Author

AntonRoskvist commented Dec 4, 2023

@jbertram
Excellent, looks good! Let me know when/if I should close this.

@jbertram
Copy link
Contributor

jbertram commented Dec 4, 2023

@AntonRoskvist, FWIW I think this was a good PR. I didn't want to ask you to make all the changes to implement what I eventually sent in #4702. However, now that I've sent it and it's all green I think you can close this one. Thanks!

@jbertram
Copy link
Contributor

jbertram commented Dec 5, 2023

@AntonRoskvist, thinking about this more I want to go with this PR rather than #4702. I like the simplicity of just enabling management for everything.

@AntonRoskvist
Copy link
Contributor Author

Alright, I'll just leave it as is then

@jbertram
Copy link
Contributor

The previous failures have been resolved, but these are failing now:

  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalQueue[useDefault=true,supportAdvisory=false,suppressJmx=false]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalAddress[useDefault=true,supportAdvisory=false,suppressJmx=false]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalQueue[useDefault=false,supportAdvisory=true,suppressJmx=false]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalAddress[useDefault=false,supportAdvisory=true,suppressJmx=false]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalQueue[useDefault=false,supportAdvisory=true,suppressJmx=true]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalAddress[useDefault=false,supportAdvisory=true,suppressJmx=true]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalQueue[useDefault=false,supportAdvisory=false,suppressJmx=true]
  • o.a.a.a.t.i.o.m.OpenWireManagementTest.testHiddenInternalAddress[useDefault=false,supportAdvisory=false,suppressJmx=true]

The problem is that we can't simply change the test here because folks really do want to ignore the management objects for OpenWire advisory resources. Therefore, I think we need to add a new queue attribute (e.g. manageable) at least for the broker to use in cases like this.

A different solution would be to leave the semantics of internal the way they are now but just change the store-and-forward queues so that they aren't internal by default and ensure that users have access to set the property on any queue via management.

Thoughts?

@AntonRoskvist
Copy link
Contributor Author

@jbertram Right, I see it now. I have added a "manageable" property to control this behavior as you suggested and I'm now running through the full test suite. If it's looking good and I have the time I'll send a new commit some time later today. Thanks!

@AntonRoskvist
Copy link
Contributor Author

@jbertram This should address the issues you noted. In addition I also set the MQTT_SESSION_STORE as "non manageable" since that is the destination that caused a few of the previous tests to fail (it always gets created if the broker starts with the mqtt-module enabled). Consequently, I reverted the changes made to queueMetrics.groovy because they are no longer needed.

@AntonRoskvist
Copy link
Contributor Author

@clebertsuconic Pinging as per your request on the dev list to consider this PR for the 2.33.0 release.
If anything's looking odd I can attempt to address it with high prio.
Thanks

@clebertsuconic
Copy link
Contributor

this is superseded by #4856

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