wu-sheng commented on a change in pull request #5407:
URL: https://github.com/apache/skywalking/pull/5407#discussion_r478938019
##########
File path:
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SegmentAnalysisListener.java
##########
@@ -153,6 +142,14 @@ public void parseSegment(SegmentObject segmentObject) {
});
final long accurateDuration = endTimestamp - startTimestamp;
duration = accurateDuration > Integer.MAX_VALUE ? Integer.MAX_VALUE :
(int) accurateDuration;
+
+ if (sampleStatus.equals(SAMPLE_STATUS.UNKNOWN) ||
sampleStatus.equals(SAMPLE_STATUS.IGNORE)) {
+ if (sampler.shouldSample(segmentObject.getTraceId()) ||
forceSaveErrorSegment) {
Review comment:
This will be always true in default, as `forceSaveErrorSegment == true`.
I think you mischanged this?
##########
File path: docs/en/setup/backend/trace-sampling.md
##########
@@ -30,4 +30,8 @@ When you set the rate different, let's say
And we assume the agents reported all trace segments to backend,
Then the 35% traces in the global will be collected and saved in storage
consistent/complete, with all spans.
20% trace segments, which reported to Backend-Instance**B**, will saved in
storage, maybe miss some trace segments,
-because they are reported to Backend-Instance**A** and ignored.
\ No newline at end of file
+because they are reported to Backend-Instance**A** and ignored.
+
+# Note
+When you open sampling, the actual sampleRate will above sampleRate. Because
we want some error segment will be saved, even that segment will abandoned by
server side trace sampling mechanism. may be miss some other trace segments,
but we can analyze this error segment to solve problem.
Review comment:
This document change should be accepted.
##########
File path:
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/AnalyzerModuleConfig.java
##########
@@ -78,4 +78,12 @@
@Getter
private final String configPath = "meter-receive-config";
+
+ /**
+ * When open sampling, true means some error segment will be saved, even
that segment will abandoned by server side trace sampling mechanism.
+ * false means all segment saved condition by server side trace sampling
mechanism.
+ */
+ @Setter
+ @Getter
+ private boolean forceSaveErrorSegment = true;
Review comment:
`forceSaveErrorSegment` -> `forceSampleErrorSegment`
##########
File path:
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SegmentAnalysisListener.java
##########
@@ -153,6 +142,14 @@ public void parseSegment(SegmentObject segmentObject) {
});
final long accurateDuration = endTimestamp - startTimestamp;
duration = accurateDuration > Integer.MAX_VALUE ? Integer.MAX_VALUE :
(int) accurateDuration;
+
+ if (sampleStatus.equals(SAMPLE_STATUS.UNKNOWN) ||
sampleStatus.equals(SAMPLE_STATUS.IGNORE)) {
+ if (sampler.shouldSample(segmentObject.getTraceId()) ||
forceSaveErrorSegment) {
Review comment:
You should `isError && forceSaveErrorSegment` from my understanding,
isn't it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
