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

Support async profiler feature #12671

Merged
merged 69 commits into from
Oct 31, 2024
Merged

Conversation

zhengziyi0117
Copy link
Contributor

Integrate async profiler performance analysis function in Java

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

@wu-sheng
Copy link
Member

The proto changes seem very concern. As those things are never merged. You should not be able to update those submodules.

Comment on lines 1 to 22
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

/*
* Copyright The async-profiler authors
* SPDX-License-Identifier: Apache-2.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

I have concerned about these dual headers. Could you confirm whether we changed this file contents? If not, we should keep their license and add exclusions for header checking.

@wu-sheng wu-sheng added this to the 10.2.0 milestone Oct 30, 2024
lujiajing1126
lujiajing1126 previously approved these changes Oct 31, 2024
Copy link
Contributor

@lujiajing1126 lujiajing1126 left a comment

Choose a reason for hiding this comment

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

LGTM! Finally all CI pass!

Great thanks to @wu-sheng, @mrproliu and of course @zhengziyi0117 for your hard work

wu-sheng
wu-sheng previously approved these changes Oct 31, 2024
@@ -0,0 +1,85 @@
`# Async Profiler
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`# Async Profiler
# Async Profiler

Comment on lines 20 to 23
Learn more tech details from the post, [**Use Profiling to Fix the Blind Spot of Distributed
Tracing**](sdk-profiling.md).

### Tracing Profiling
Copy link
Member

Choose a reason for hiding this comment

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

This part should be kept in Tracing Profiling.

@kezhenxu94
Copy link
Member

kezhenxu94 commented Oct 31, 2024

2 comments of mine are still not addressed, you can merge this as you wish but make sure to address later or explain why they can’t be addressed:

  1. Support async profiler feature #12671 (comment)
  2. Support async profiler feature #12671 (comment)

Copy link
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.

Considering OSPP deadline and this feature is mostly good. I am going to merge this.

@wu-sheng
Copy link
Member

2 comments of mine are still not addressed, you can merge this as you wish but make sure to address later or explain why they can’t be addressed:

  1. Support async profiler feature #12671 (comment)
  2. Support async profiler feature #12671 (comment)

@zhengziyi0117 Please address this in a new pull request. We need to make this 100% ready, because this will be in SkyWalking next major feature release.

@wu-sheng wu-sheng merged commit 9e5fb8f into apache:master Oct 31, 2024
165 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. complexity:medium Relate to multiple(>1 & <4) components of SkyWalking feature New feature profiling SDK profiling and eBPF profiling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants