SkyAPM / go2sky

Distributed tracing and monitor SDK in Go for Apache SkyWalking APM
https://skywalking.apache.org/
Apache License 2.0
448 stars 122 forks source link

Adding custom report filter #163

Closed JJ-Jasmin closed 1 year ago

JJ-Jasmin commented 1 year ago

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

wu-sheng commented 1 year ago

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.

JJ-Jasmin commented 1 year ago

for example: only submit requests below 100ms

wu-sheng commented 1 year ago

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 commented 1 year ago

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

JJ-Jasmin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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%) :arrow_down:
reporter/grpc_opts.go 71.42% <0.00%> (-5.50%) :arrow_down:
sampler.go 81.96% <0.00%> (+3.27%) :arrow_up:

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

JJ-Jasmin commented 1 year ago

Has been updated