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

KAFKA-18390: Use LinkedHashMap instead of Map in creating MetricName and SensorBuilder (1/N) #19300

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

TaiJuWu
Copy link
Collaborator

@TaiJuWu TaiJuWu commented Mar 27, 2025

JIRA: https://issues.apache.org/jira/browse/KAFKA-18390

This is the first part related SensorBuilder.

In this PR, changed SensorBuilder(Metrics metrics, String name, Supplier<Map<String, String>> tagsSupplier) to public SensorBuilder(Metrics metrics, String name, Supplier<LinkedHashMap<String, String>> tagsSupplier)

@github-actions github-actions bot added triage PRs from the community consumer clients small Small PRs labels Mar 27, 2025
@TaiJuWu TaiJuWu requested a review from chia7712 March 27, 2025 11:09
@dajac
Copy link
Member

dajac commented Mar 27, 2025

@TaiJuWu Thanks for the patch. Out of curiosity, have we checked that jmx metric names are not altered by this change? I think that they are generated based on the order in the data structure.

@TaiJuWu
Copy link
Collaborator Author

TaiJuWu commented Mar 28, 2025

@TaiJuWu Thanks for the patch. Out of curiosity, have we checked that jmx metric names are not altered by this change? I think that they are generated based on the order in the data structure.

Hi @dajac , thanks for your review.
I thinks the order is not changed. Both of them are following.
Screenshot 2025-03-28 at 13 02 25

But I am a little confused why we need to care jmx metrics name?
In the past, we use HashMap so it is unordered but we use LinkedHashMap in this PR. The former is non-deterministic and dependent on different implementation but the latter is deterministic.

If there is any user assume metrics order are deterministic, that is an issue they need to fix.
If I misunderstood anything, please correct me, thanks.

Another thing is a PR related MetricName #19222, it is difficult if we want to check all order is same as before.
We can use follow script to check the order.

@TaiJuWu
Copy link
Collaborator Author

TaiJuWu commented Mar 28, 2025

I use following command to list metrics with/without this PR and diff them to check the order.

#!/bin/bash

# set Kafka JMX port(need to set by manual)
JMX_PORT=9999
JMXTERM_JAR="jmxterm.jar"

if [ ! -f "$JMXTERM_JAR" ]; then
    echo "🔍 jmxterm.jar is not existing,downloading..."
    wget -q https://github.com/jiaqi/jmxterm/releases/download/v1.0.2/jmxterm-1.0.2-uber.jar -O "$JMXTERM_JAR"
    echo "✅ jmxterm.jar download finish"
fi

# generate jmxterm command
CMD=$(cat <<EOF
open localhost:$JMX_PORT
beans
EOF
)

echo "🔍 try getting Kafka Consumer 的 JMX metric..."
MBeans=$(echo "$CMD" | java -jar "$JMXTERM_JAR" | awk '{print $1}')

# iterator Kafka Consumer MBean, list all metrics name
for bean in $MBeans; do
    echo "----------------------------------------"
    echo "📌 Kafka Consumer JMX metric: $bean"
    CMD=$(cat <<EOF
open localhost:$JMX_PORT
domain kafka.consumer
bean $bean
info
EOF
    )
    echo "$CMD" | java -jar "$JMXTERM_JAR" | grep " - " | sed 's/ - /  🔹 /g'
done

echo "✅ Kafka Consumer JMX read finish!"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients consumer small Small PRs triage PRs from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants