-
Couldn't load subscription status.
- Fork 6.6k
feat(add go profile data collect and analyze) #13524
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
base: master
Are you sure you want to change the base?
Conversation
|
Please run the CI locally. I noticed you are using the upstream resources and the maintainer's manual approval to test. This is not efficiency. Please set up the environment locally and make sure they are passed locally. |
.github/workflows/skywalking.yaml
Outdated
| config: test/e2e-v2/cases/profiling/trace/opensearch/e2e.yaml | ||
| env: OPENSEARCH_VERSION=2.4.0 | ||
|
|
||
| - name: Go Profiling |
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.
| - name: Go Profiling | |
| - name: Go Trace Profiling |
.github/workflows/skywalking.yaml
Outdated
| env: OPENSEARCH_VERSION=2.4.0 | ||
|
|
||
| - name: Go Profiling | ||
| config: test/e2e-v2/cases/go/profiling-cases.yaml |
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 move this into test/e2e-v2/profiling/trace/go folder.
| with: | ||
| e2e-file: $GITHUB_WORKSPACE/test/e2e-v2/cases/simple/jdk/e2e.yaml | ||
|
|
||
| # e2e-test-banyandb-stages: |
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.
Why you need to change it?
.licenserc.yaml
Outdated
| - '**/mockito-extensions/**' | ||
| - 'oap-server/server-library/library-async-profiler-jfr-parser' | ||
| - 'docs/en/changes/changes.tpl' | ||
| - 'oap-server/server-core/src/main/proto/profile.proto' |
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 should not have this file.
| @ElasticSearch.EnableDocValues | ||
| @Column(name = IS_GO) | ||
| @BanyanDB.NoIndexing | ||
| private int isGo; // store as 0/1 for storage compatibility |
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 change the field name as language type.
| * Analyze a pprof profile for a specific segment and time window. | ||
| * periodMs: derived from pprof period/periodType; fallback to 10ms when absent. | ||
| */ | ||
| public ProfileAnalyzation analyze(final String segmentId, |
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.
Is this already done in the proof library?
| <version>${project.version}</version> | ||
| </dependency> | ||
| <!-- Protobuf runtime for generated classes in this module --> | ||
| <dependency> |
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.
Import the pprof library is enough.
| - e2e | ||
| expose: | ||
| - 8080 | ||
| ports: |
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.
Why you need to define this? Please remove it encase its works for the debug.
| - id: "{{ notEmpty .id }}" | ||
| parentid: "{{ notEmpty .parentid }}" | ||
| codesignature: "test.apache.skywalking.e2e.profile.ProfileController.createAuthor:-1" | ||
| codesignature: "{{ notEmpty .codesignature }}" |
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.
Why you need to update this?
|
|
||
| tip: null | ||
| trees: | ||
| {{- if .trees }} |
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 rollback to the original one.
|
I am still seeing the GHA flow control files are changed, as well as UI submodule. @mrproliu Please make sure the students understand my asking. And run locally rather than keeping costing upstream. |
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 should not update the protocol, the protocol has already been updated to the latest version.
docs/en/changes/changes.md
Outdated
| * Support the go agent(0.7.0 release) bundled pprof profiling feature. | ||
|
|
||
| * Profile-receiver-plugin: feat: add go profile data receive func; add google pprof proto to prase. | ||
| * Storage: feat: add isGo column to ProfileThreadSnapshotRecord. |
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 column name should be changed.
| return languageType == Language.GO.getValue(); | ||
| } | ||
|
|
||
| public void setGo(final boolean go) { |
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.
It's not suitable for a new language in the future, please remove it.
| segmentId, start, end); | ||
|
|
||
| // Go-only fallback: if this segment has Go snapshots, ignore time window and fetch any sequences | ||
| int fbMin = getProfileThreadSnapshotQueryDAO().queryMinSequence(segmentId, 0L, Long.MAX_VALUE); |
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.
Why can it not use the previous query result in this method? In your way, it will make another 2 DB queries.
| try { | ||
| // Parse pprof format profile data (payload may be gzip-compressed per pprof spec) | ||
| byte[] parsedBytes = tryDecompressGzip(profileData); | ||
| ProfileProto.Profile profile = ProfileProto.Profile.parseFrom(parsedBytes); |
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 use library-pprof-parser for this.
-2025-10-29-16-37-03.MP4 |
| FrameTree tree = new FrameTree(getSignature(rawTree.getLocationId()), rawTree.getTotal(), rawTree.getSelf()); | ||
| FrameTree tree = new FrameTree(getSignature(rawFrameTreeGetLocationId(rawTree)), rawTree.getTotal(), rawTree.getSelf()); | ||
| for (RawFrameTree rawChild : rawTree.getChildren().values()) { | ||
| FrameTree child = parseTree(rawChild); | ||
| tree.getChildren().add(child); | ||
| } | ||
| return tree; | ||
| } | ||
|
|
||
| // Small indirection to keep minimal change footprint while delegating signature resolution | ||
| private long rawFrameTreeGetLocationId(RawFrameTree rawTree) { | ||
| return rawTree.getLocationId(); | ||
| } |
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.
hi, why do you want to add rawFrameTreeGetLocationId method, it seems that there is no change here
CHANGESlog.