Skip to content

Support to customize the collect period of JVM relative metrics.#428

Merged
wu-sheng merged 5 commits intoapache:mainfrom
IceSoda177:main
Dec 26, 2022
Merged

Support to customize the collect period of JVM relative metrics.#428
wu-sheng merged 5 commits intoapache:mainfrom
IceSoda177:main

Conversation

@IceSoda177
Copy link
Copy Markdown
Contributor

@IceSoda177 IceSoda177 commented Dec 22, 2022

Support to customize the collect period of JVM relative metrics.

@nisiyong
Copy link
Copy Markdown
Contributor

It seems the OAP side will calculate GC time per second. This change should consider more.

/**
* The period in seconds of JVM metrics collection.
*/
public static int METRICS_COLLECT_PERIOD = 10;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should not change the default value.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is rarely to see thousands threads for a health service.

Comment thread apm-sniffer/config/agent.config Outdated
@wu-sheng
Copy link
Copy Markdown
Member

It seems the OAP side will calculate GC time per second. This change should consider more.

Could you list which metrics?

@wu-sheng wu-sheng added the enhancement New feature or request label Dec 22, 2022
@IceSoda177
Copy link
Copy Markdown
Contributor Author

It seems the OAP side will calculate GC time per second. This change should consider more.

I don't see any necessary gc metrics calculation for every second.
The OAP use sum() method in OAL to aggregate gc metrcis, including time and count of different gc phase.
After that, it will generat the final gc metrics of 1min.

On the other hand, Agent record lastest time or count of diff gc phase, and use them to create new metrics in next time collection.
So, I think it's fine if anyone want to change their JVM metrics collect period

@IceSoda177
Copy link
Copy Markdown
Contributor Author

You should not change the default value.

My bad

@IceSoda177 IceSoda177 marked this pull request as draft December 22, 2022 10:31
@IceSoda177 IceSoda177 marked this pull request as ready for review December 22, 2022 10:38
@IceSoda177
Copy link
Copy Markdown
Contributor Author

You should not change the default value.

The default value stays and the configuration.md is updated.
Excuse me for my first pr. ^-^

@wu-sheng
Copy link
Copy Markdown
Member

Let's wait for @nisiyong feedback. And the CI would fail,

#427 (comment)

I need to wait for both things.

@nisiyong
Copy link
Copy Markdown
Contributor

I may have just got COVID-19, I will check it when I get better😿

@wu-sheng
Copy link
Copy Markdown
Member

wu-sheng commented Dec 22, 2022

I may have just got COVID-19, I will check it when I get better😿

Take care. I am suffering symptoms too. 4 days fever, but it is going down.

@IceSoda177
Copy link
Copy Markdown
Contributor Author

IceSoda177 commented Dec 23, 2022

Tack care guys.
I'm lucky so far, but we really out of meds here.

Comment thread CHANGES.md
* Clean the trace context which injected into Pulsar MessageImpl after the instance recycled
* Fix In the higher version of mysql-connector-java 8x, there is an error in the value of db.instance.
* Add support for KafkaClients 3.2.x
* Add support for KafkaClients 3.x.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lujiajing1126 I made this. As #429 missed to correct versions.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I forgot to fix it there

@nisiyong
Copy link
Copy Markdown
Contributor

It seems the OAP side will calculate GC time per second. This change should consider more.

@dylanforest @wu-sheng Sorry for my late reply, this PR looks good to me.

And I should correct what I said before, the agent side will calculate the GC time rather than the OAP side.

Currently, the metrics are stored at a minute level in the backend, so I think this is fine.

@wu-sheng
Copy link
Copy Markdown
Member

It seems the OAP side will calculate GC time per second. This change should consider more.

@dylanforest @wu-sheng Sorry for my late reply, this PR looks good to me.

And I should correct what I said before, the agent side will calculate the GC time rather than the OAP side.

Currently, the metrics are stored at a minute level in the backend, so I think this is fine.

Thanks for the clarification.

Currently, the metrics are stored at a minute level in the backend

About this, I want to add a little more. The metrics are SUMed in minute/hour/day. From the visualization part, it shows VM GC Time (ms) and JVM GC Count in the time buckets of the range.


I agree, this change should be fine.

@wu-sheng
Copy link
Copy Markdown
Member

BTW @nisiyong Are you feeling better now?

@wu-sheng wu-sheng added this to the 8.14.0 milestone Dec 26, 2022
Comment thread docs/en/setup/service-agent/java-agent/configurations.md Outdated
Comment thread apm-sniffer/config/agent.config Outdated
Copy link
Copy Markdown
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dylanforest I added Unit is second. to the doc and config, to make the new config more clear.

@nisiyong
Copy link
Copy Markdown
Contributor

BTW @nisiyong Are you feeling better now?

Thanks for your concern, I am much better now. Had a fever for 3 days and it was really painful.

@wu-sheng
Copy link
Copy Markdown
Member

BTW @nisiyong Are you feeling better now?

Thanks for your concern, I am much better now. Had a fever for 3 days and it was really painful.

Same here. 3 days fever. Take enough rest. This doesn't go away immediately. The fever could be back.

@wu-sheng wu-sheng merged commit a68b447 into apache:main Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants