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

Adding custom report filter #163

Merged
merged 11 commits into from
Jul 28, 2022
Merged

Adding custom report filter #163

merged 11 commits into from
Jul 28, 2022

Conversation

JJ-Jasmin
Copy link
Contributor

Sometimes we need a custom filter to filter the reported information to reduce the storage pressure

 Sometimes we need a custom filter to filter the reported information to reduce the storage pressure
Sometimes we need a custom filter to filter the reported information to reduce the storage pressure
Adding custom report filter
@wu-sheng
Copy link
Member

Storage pressure should be taken care of by backend OAP capability.
If you don't want to trace at the beginning, a sampler makes a more sense before creating context.

@wu-sheng wu-sheng requested a review from arugal July 27, 2022 11:24
@JJ-Jasmin
Copy link
Contributor Author

for example: only submit requests below 100ms

@wu-sheng
Copy link
Member

That is actually very tricky. You could make a lot of VNode when visuals trace.
Sampling usually is a tradeoff, but your point of reducing storage doesn't make sense to provide this feature.

@JJ-Jasmin
Copy link
Contributor Author

some of the problems just mentioned should actually reduce the pressure on grpc to report

@JJ-Jasmin
Copy link
Contributor Author

In the case of high QPS, grpc server needs a lot of resources. In fact, we can only collect and analyze requests with more than 100ms or errors

@wu-sheng
Copy link
Member

But your way as post sampling actually could make things not as expected in the runtime.
When your service has performance impact, the sampler would report more and more segments, which slows the service more.
This is not a good practice in prod.

@wu-sheng
Copy link
Member

In the case of high QPS, grpc server needs a lot of resources. In fact, we can only collect and analyze requests with more than 100ms or errors

How many QPS are you talking about? Do you really test the tracing cost VS reporting cost?

In most languages, tracing cost is almost 5-10 times than pure reporting.

@JJ-Jasmin
Copy link
Contributor Author

In fact, the problem we encounter is that in 50000qps requests, we only need to track those wrong and slow requests, and the vast majority of requests (99.9%) do not need to be submitted to the tracking. If the full number of grpc submissions, the server received by grpc may need more resources, so we want to filter out (retain wrong and slow requests) when submitting to reduce the pressure of grpc reporting. Do you have a better approach?

@wu-sheng
Copy link
Member

But that is not SkyWalking's typical use case. I am actually confused about your decision.
If you just need some traces of error or slow, and don't care about the analysis/metrics, why do you choose SkyWalking rather than many others out there?
SkyWalking's traces are designed for dependencies and metrics analysis, but it seems this is also not your purpose.
Then why do you choose SkyWalking, but use it like a pure tracing stack?

@wu-sheng wu-sheng added the enhancement New feature or request label Jul 27, 2022
@wu-sheng
Copy link
Member

Back to the codes, I think you need to update the doc at https://github.com/SkyAPM/go2sky#configuration about how to use this.

We need the doc to explain what is CustomReportStrategy. But first, this should be renamed to ReportStrategy.(Custom is not necessary).

@wu-sheng wu-sheng added this to the 1.5.0 milestone Jul 27, 2022
README.md Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

@arugal Could you review the codes? Although I am not supporting this use case, this seems harmless either.

I will leave the judgment for you.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

Merging #163 (282c26a) into master (6492c49) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   64.64%   64.53%   -0.12%     
==========================================
  Files          22       22              
  Lines        1089     1094       +5     
==========================================
+ Hits          704      706       +2     
- Misses        330      334       +4     
+ Partials       55       54       -1     
Impacted Files Coverage Δ
reporter/grpc.go 50.68% <0.00%> (-0.47%) ⬇️
reporter/grpc_opts.go 71.42% <0.00%> (-5.50%) ⬇️
sampler.go 81.96% <0.00%> (+3.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@JJ-Jasmin
Copy link
Contributor Author

Has been updated

reporter/grpc.go Outdated Show resolved Hide resolved
remove default report strategy
remove default report strategy
Copy link
Member

@arugal arugal left a comment

Choose a reason for hiding this comment

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

LGTM

@arugal arugal merged commit 0ce76f9 into SkyAPM:master Jul 28, 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