You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "naude-r (via GitHub)" <gi...@apache.org> on 2023/05/25 07:49:42 UTC
[GitHub] [activemq-artemis] naude-r opened a new pull request, #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
naude-r opened a new pull request, #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487
Add the "broker" tag in a consistent manner to all exported metrics. This is needed to allow plugins to
export additional metrics with the broker tag. Some scrapers, e.g. prometheus, add an "instance" tag. This value may not be the same as the broker name, which results in these metrics becoming more difficult to match up with the corresponding broker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205828147
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
In my opinion the broker should simply support the processor and uptime metrics just like it does the JVM and Netty metrics. That way _every_ user could benefit from them and the plugin interface wouldn't need to change. I created https://issues.apache.org/jira/browse/ARTEMIS-4292 for this work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r closed pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r closed pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
URL: https://github.com/apache/activemq-artemis/pull/4487
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1569040368
Yes, that makes sense. Before I merge it can you add a test to ensure that the {{broker}} tag is set on the JVM & Netty metrics as expected? You could modify {{org.apache.activemq.artemis.tests.integration.plugin.JvmMetricsTest}} for the JVM metrics. However, we don't have an existing test for the Netty metrics so you could copy the aforementioned {{JvmMetricsTest}} and modify it for the Netty use-case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205610027
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
sure. we have a timing issue in that ActiveMQMetricsPlugin::init is called before MetricsManager have had the chance to add the common tags.
any metrics created / registered before then will not have the "broker" tag.
not sure there is an easy way to do that without breaking existing plugins?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1570696276
I just sent #4497 to deal with the metrics you were adding manually before. Once merged & released you can use `broker.xml` to configure those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1570714527
BTW, thanks for the contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205829410
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
Back this change out and I'll merge it (assuming it passes the PR tests). Then I'll send a PR for ARTEMIS-4292.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1569549272
> Yes, that makes sense. Before I merge it can you add a test to ensure that the {{broker}} tag is set on the JVM & Netty metrics as expected? You could modify {{org.apache.activemq.artemis.tests.integration.plugin.JvmMetricsTest}} for the JVM metrics. However, we don't have an existing test for the Netty metrics so you could copy the aforementioned {{JvmMetricsTest}} and modify it for the Netty use-case.
>
> To be clear, just about every commit needs a test to validate the change is working as expected and to mitigate against future regressions.
certainly. will get to it either today or tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1563857150
consider the following two metrics as scraped by prometheus:
```
artemis_address_size{address="ExpiryQueue",broker="$FQDN",endpoint="metrics-port",instance="$IP:8161",job="artemis",namespace="infra",service="artemis-exporters"}
jvm_info{endpoint="metrics-port",instance="$IP:8161",job="artemis",namespace="infra",runtime="OpenJDK Runtime Environment",service="artemis-exporters",vendor="Red Hat, Inc.",version="17.0.5+8-LTS"}
```
the only way to link the two metrics are the "instance" tag, which in our case is a raw ip (broker running external to k8s and k8s endpoints only supports ip addresses). the "broker" contains a user friendly name which make the dashboard easier to use.
in my opinion there is no way that adding a "broker" tag can break any existing dashboards and/or expectation. the cardinality will also not increase since the same broker tag/value pair is already available from the address metrics.
hope that make sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205636166
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
Are you adding your _own_ metrics in your plugin implementation?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram merged pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram merged PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205770107
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
yes,
we do:
```
new ProcessorMetrics().bindTo(meterRegistry);
new UptimeMetrics().bindTo(meterRegistry);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1563272804
Please reformat your commit message to match [our standards](https://github.com/apache/activemq-artemis/blob/main/docs/hacking-guide/en/code.md#commitMessageDetails). Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1569907193
> > Yes, that makes sense. Before I merge it can you add a test to ensure that the {{broker}} tag is set on the JVM & Netty metrics as expected? You could modify {{org.apache.activemq.artemis.tests.integration.plugin.JvmMetricsTest}} for the JVM metrics. However, we don't have an existing test for the Netty metrics so you could copy the aforementioned {{JvmMetricsTest}} and modify it for the Netty use-case.
> > To be clear, just about every commit needs a test to validate the change is working as expected and to mitigate against future regressions.
>
> certainly. will get to it either today or tomorrow.
test cases added as requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1563429260
I'm concerned this may break existing users since the name of the JVM and Netty metrics are doing to change now. Can you speak to this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205843931
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
thanks. done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a diff in pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#discussion_r1205590953
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/ActiveMQMetricsPlugin.java:
##########
@@ -26,4 +26,10 @@ public interface ActiveMQMetricsPlugin extends Serializable {
ActiveMQMetricsPlugin init(Map<String, String> options);
MeterRegistry getRegistry();
+
+ /**
+ * Allow additional binders to be bound at the right time
+ */
+ default void bind() {
Review Comment:
It's not clear why this new method is needed. Can you elaborate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] naude-r commented on pull request #4487: ARTEMIS-4291 Add the "broker" tag in a consistent manner
Posted by "naude-r (via GitHub)" <gi...@apache.org>.
naude-r commented on PR #4487:
URL: https://github.com/apache/activemq-artemis/pull/4487#issuecomment-1571402949
Thank you for the additional metrics!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org