Skip to content
This repository has been archived by the owner on Jun 14, 2023. It is now read-only.

Add process metadata file for work with eBPF agent #156

Merged
merged 7 commits into from
Apr 17, 2022

Conversation

mrproliu
Copy link
Contributor

Now we have skywalking-rover, which could detect processes from the local machine. Also, go2sky is one of the official agents in the skywalking ecosystem. We could let these two projects cooperate. If the local machine already has service use go2sky, then the rover could find this process automatically. Finally, the rover side could collect, profiling, etc.

In principle, when go2sky performs keep alive with the backend, it would write a metadata file to the local (temporary directory) at the same time, which describes the information of the current process. The Rover side scans all processes, find out which processes contain this metadata file, and update the confirmed files. Finally, if go2sky sees that it has been confirmed, it would not modify the files again.

@mrproliu mrproliu added the enhancement New feature or request label Apr 15, 2022
@mrproliu mrproliu added this to the 1.4.1 milestone Apr 15, 2022

// Report the current process metadata to local file
// using to work with eBPF agent
func reportProcessIFNeed(r *gRPCReporter) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need a very clear option for this new feature. Labels are optional, this should not

Copy link
Member

Choose a reason for hiding this comment

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

And please document clearly what file(s) is being created, and which information would be written.
Then when the rover side doc is ready, this should link to there for further information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need a very clear option for this new feature. Labels are optional, this should not

Yes, this function is not optional, maybe the function name let you feel confused. updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And please document clearly what file(s) is being created, and which information would be written. Then when the rover side doc is ready, this should link to there for further information.

Updated. After this merge, I will update the documentation on the rover side and finally link the document there.

@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2022

Codecov Report

Merging #156 (9d9c061) into master (2d02004) will decrease coverage by 6.19%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   71.02%   64.83%   -6.20%     
==========================================
  Files          21       22       +1     
  Lines         994     1089      +95     
==========================================
  Hits          706      706              
- Misses        236      329      +93     
- Partials       52       54       +2     
Impacted Files Coverage Δ
reporter/grpc.go 51.15% <0.00%> (-0.72%) ⬇️
reporter/grpc_env.go 45.45% <0.00%> (-14.55%) ⬇️
reporter/grpc_opts.go 76.92% <ø> (-23.08%) ⬇️
reporter/process.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d02004...9d9c061. Read the comment docs.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -264,5 +264,36 @@ Golang agent supports the following dynamic configurations.
|:-----------------:|:----------------------------------------------------------------------------------------:|:--------------------:|
| agent.sample_rate | The percentage of trace when sampling. It's `[0, 1]`, Same with `WithSampler` parameter. | 0.1 |

## Process

This feature is used in cooperation with the [skywalking-rover](https://github.com/apache/skywalking-rover) project.
Copy link
Member

Choose a reason for hiding this comment

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

I think you missed one, I suggest this feature(temp files) should be optional and OFF on default.

Copy link
Member

Choose a reason for hiding this comment

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

It is recommended to enable this feature through environment variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and let the process label and process status hook enabled could update through the environment variable.

@wu-sheng
Copy link
Member

I think 1.4.1 is released, this PR is in the wrong milestone.

@arugal Do we need plugin 1.4.1 release?

@wu-sheng wu-sheng modified the milestones: 1.4.1, 1.5.0 Apr 17, 2022
Comment on lines +273 to +274
When go2sky keeps alive with the backend, it would write a metadata file to the local (temporary directory) at the same time, which describes the information of the current process.
The rover side scans all processes, find out which process contains this metadata file. Finally, the rover could collect, profiling, with this process.
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
When go2sky keeps alive with the backend, it would write a metadata file to the local (temporary directory) at the same time, which describes the information of the current process.
The rover side scans all processes, find out which process contains this metadata file. Finally, the rover could collect, profiling, with this process.
When go2sky keeps alive with the backend, it would write a metadata file to the local (temporary directory) at the same time, which provides the information of the current process.
The rover would detect this file in order to identify this Golang service, and tags it `ebpf-profiling-able`.

As a doc, we don't need to describe how codes work, they are open sourced :) We need to tell why it works in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ebpf-profiling-able? Do you mean to add this tag to the process labels?

Copy link
Member

Choose a reason for hiding this comment

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

I mean logically, this is what this is designed for. Nothing about label.

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.

Waiting for @arugal 's approval.

@arugal
Copy link
Member

arugal commented Apr 17, 2022

@arugal Do we need plugin 1.4.1 relea se?

Yes, I left out

@arugal arugal merged commit 216f122 into SkyAPM:master Apr 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants