-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][metrics]wrong metrics text generated when label_cluster specified #17704
Conversation
@poorbarcode Please provide a correct documentation label for your PR. |
188b1a4
to
e9ad9c9
Compare
This PR should merge into the following branches:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -67,14 +68,21 @@ public static void generateSystemMetrics(SimpleTextOutputStream stream, String c | |||
stream.write(sample.name); | |||
if (!sample.labelNames.contains("cluster")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can add this stream.write("{");
before the check if (!sample.labelNames.contains("cluster"))
. The symbol {
is necessary and it's easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. The code has been changed
2980fdb
to
3023772
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
import java.util.Collections; | ||
import java.util.UUID; | ||
import org.testng.annotations.Test; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the test group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already add test group
3023772
to
bd65de8
Compare
/pulsarbot rerun-failure-checks |
/pulsarbot rerun-failure-checks |
Hi @Technoboy- Can this PR merge? (^_^) |
…ster specified (#17704) (#18919) Co-authored-by: fengyubiao <[email protected]>
…ster specified (apache#17704) (apache#18919) Co-authored-by: fengyubiao <[email protected]> (cherry picked from commit a530ab2)
…ster specified (apache#17704) (apache#18919) Co-authored-by: fengyubiao <[email protected]> (cherry picked from commit a530ab2)
…ed (#17704) * [fix][metrics]wrong metrics text generated when label_cluster specified * improve logic branch * mark test group
Motivation
If
label_cluster
specifiedcluster_123
, the expected metrics text is:but the actual metrics text is :
High light: lost a char
{
Modifications
Make metrics text generate correct
Documentation
doc-required
doc-not-needed
doc
doc-complete
Matching PR in forked repository
PR in forked repository: