-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Add some new thread metric and class metric to JVMMetric (#7230) #7243
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
Add some new thread metric and class metric to JVMMetric (#7230) #7243
Conversation
…o feature/agent/new_jvm_metric
…o feature/agent/new_jvm_metric
Associate #7230 and apache/skywalking-data-collect-protocol#52 |
|
@mrproliu I think we will need to update satellite and goapi for the latest JVM protocol. |
…ic to JVMMetric. (apache#7230)
At least add one of each kind metrics into e2e testing, we need an automatic process to verify your changes are working. |
Ok, I try write some e2e testcase. @wu-sheng Is the tutorial End to End Tests (E2E)? |
Codecov Report
@@ Coverage Diff @@
## master #7243 +/- ##
============================================
- Coverage 59.02% 58.57% -0.45%
- Complexity 4342 4349 +7
============================================
Files 1020 1032 +12
Lines 26256 26506 +250
Branches 2600 2622 +22
============================================
+ Hits 15497 15526 +29
- Misses 9389 9609 +220
- Partials 1370 1371 +1
Continue to review full report at Codecov.
|
|
The basic processes of e2e are
|
Follow the GitHub Action control file, it should not hard to understand, if you have known maven, unit test, and docker/docker-compose |
I think this isn't hard, but i need some time learning it. |
Oh, you don't need to write a new one. There are many existing cases which is verifying JVM-related metrics. You just need to add one metric about thread, the other about class loaded to verify the codes. |
Ok, I get it. |
ThreadInfo[] threadInfos = threadMXBean.getThreadInfo(threadMXBean.getAllThreadIds(), 0); | ||
if (threadInfos != null) { | ||
for (ThreadInfo threadInfo : threadInfos) { | ||
if (threadInfo != null) { | ||
switch (threadInfo.getThreadState()) { | ||
case NEW: | ||
newStateThreadCount++; | ||
break; | ||
case RUNNABLE: | ||
runnableStateThreadCount++; | ||
break; | ||
case BLOCKED: | ||
blockedStateThreadCount++; | ||
break; | ||
case WAITING: | ||
waitingStateThreadCount++; | ||
break; | ||
case TIMED_WAITING: | ||
timedWaitingStateThreadCount++; | ||
break; | ||
case TERMINATED: | ||
terminatedStateThreadCount++; | ||
break; | ||
default: | ||
break; | ||
} | ||
} else { | ||
terminatedStateThreadCount++; | ||
} |
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.
The code as a whole looks good. However, there may be some inaccuracies in this place. I found that getAllThreadIds()
and getThreadInfo()
method both return information about the active thread. However, when the thread is in the NEW and TERMINATED state, the isAlive()
method will return false, that is, they are inactive. Therefore, in this way, we cannot correctly count the number of threads in the NEW and TERMINATED state.
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.
So we need to consider whether we want to remove them.
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.
So we need to consider whether we want to remove them.
New State
is zero always, but Terminated State
is non-zero sometimes.
So, I should remove them?
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.
If there is no threadInfo, I think we should not run statistics for it.
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.
If you remove the counting logic from the else code block, you may find that the Terminated State
is zero always.
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.
If you remove the counting logic from the else code block, you may find that the
Terminated State
is zero always.
Of course.
So, I remove Terminated State
and New State
later.
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.
If you remove the counting logic from the else code block, you may find that the
Terminated State
is zero always.Of course.
So, I remove
Terminated State
andNew State
later.
I remove it.
Co-authored-by: Ax1an <ax1an@foxmail.com>
Should we remove them from the protocol as well? |
Yes, I think so. |
So, need I remove them? |
Yes. |
If you are not going to report them, then we remove them. The protocol should include necessary fields |
Ok, I will create PR to remove them. |
I am not sure why |
@Switch-vov The following information is for your reference. |
serviceInstanceJVMThread.setRunnableStateThreadCount(thread.getRunnableStateThreadCount()); | ||
serviceInstanceJVMThread.setBlockedStateThreadCount(thread.getBlockedStateThreadCount()); | ||
serviceInstanceJVMThread.setWaitingStateThreadCount(thread.getWaitingStateThreadCount()); | ||
serviceInstanceJVMThread.setTimedWaitingStateThreadCount(thread.getTimedWaitingStateThreadCount()); |
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.
I think this would break compatibility for all previous java agent. We need to avoid this.
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.
You may use hasField
to determine the version.
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.
You may use
hasField
to determine the version.
What means hasField
?
Read the log, I found the first metrics Total Class Loader
is zero always.
So, What should I do?
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.
The compatibility test is not using the latest agent, it checks the compatibility with the previous release. I remember it uses 8.2 or 8.1 release agent. In that agent, you don't have these metrics. You should make that e2e test verifier doesn't check these added metrics.
My point was, you may need to check whether has errors here because the data OAP received in this case, they even don't report all these fields. Now, I think you could ignore my comments about You may use hasField to determine the version.
, protobuf should have covered this case, and focus on different metrics verification in different test cases.
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.
The compatibility test is not using the latest agent, it checks the compatibility with the previous release. I remember it uses 8.2 or 8.1 release agent. In that agent, you don't have these metrics. You should make that e2e test verifier doesn't check these added metrics.
My point was, you may need to check whether has errors here because the data OAP received in this case, they even don't report all these fields. Now, I think you could ignore my comments about
You may use hasField to determine the version.
, protobuf should have covered this case, and focus on different metrics verification in different test cases.
So, should I delete there test-case of metrics in e2e?
In addition, What need I do about 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.
@kezhenxu94 Do we have a chance to verify different metrics in the compatible case?
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.
Yes. @Switch-vov don’t share compatibility test class with simple case, copy SimpleE2E
class for compatibility (CompatE2E
) and replace
test_class: org.apache.skywalking.e2e.simple.SimpleE2E |
CompatE2E
class. Then in CompatE2E
don’t verify those new metrics.
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.
@Switch-vov Please follow ☝️
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.
OK
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.
@Switch-vov Please follow ☝️
I'm try to refactor SimpleE2E, CompatE2E and KafkaE2E.
…tch-vov/skywalking into feature/agent/new_jvm_metric
Before run CI, @kezhenxu94 Could you recheck the CI codes change? Are they expected? I can see inheritance level change happens. |
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.
Before run CI, @kezhenxu94 Could you recheck the CI codes change? Are they expected? I can see inheritance level change happens.
The changes look good to me. Some shared codes are extracted into a class and others extend from it.
test/e2e/e2e-test/src/test/java/org/apache/skywalking/e2e/simple/SimpleE2EBase.java
Outdated
Show resolved
Hide resolved
…le/SimpleE2EBase.java
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.
Generally good to me, wait for others' review. @Switch-vov Could you post a system screenshot to see what does the latest UI look like?
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
OK,That's all screenshot. @wu-sheng |
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.
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
CHANGES
log.