You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2022/10/28 02:28:11 UTC
[GitHub] [skywalking] mrproliu opened a new pull request, #9857: Support `slowTrace` in LAL
mrproliu opened a new pull request, #9857:
URL: https://github.com/apache/skywalking/pull/9857
* Support `slowTrace` statement in LAL and add some configuration for the network profiling analysis. Since the network profiling in Rover may analysis processes from different layers, multiple LAL configurations need to be added.
* Support multiple rules with different names under the same layer of LAL script.
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Part of https://github.com/apache/skywalking/issues/9802.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `sampleTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008603137
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/LogFilterListener.java:
##########
@@ -40,27 +41,29 @@
@Slf4j
@RequiredArgsConstructor
public class LogFilterListener implements LogAnalysisListener {
- @lombok.NonNull
- private final DSL dsl;
+ private final Collection<DSL> dsls;
@Override
public void build() {
- try {
- dsl.evaluate();
- } catch (final Exception e) {
- log.warn("Failed to evaluate dsl: {}", dsl, e);
- }
+ dsls.forEach(dsl -> {
+ try {
+ dsl.evaluate();
+ } catch (final Exception e) {
+ log.warn("Failed to evaluate dsl: {}", dsl, e);
+ }
+ });
}
@Override
public LogAnalysisListener parse(final LogData.Builder logData,
Review Comment:
*[CanIgnoreReturnValueSuggester](https://errorprone.info/bugpattern/CanIgnoreReturnValueSuggester):* Methods that always 'return this' should be annotated with @CanIgnoreReturnValue
---
```suggestion
@CanIgnoreReturnValue
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=349148748&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349148748&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349148748&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349148748&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349148748&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008787694
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampledTraceBuilder sampledTraceBuilder() {
+ return (SampledTraceBuilder) getProperty(KEY_SAMPLED_TRACE);
+ }
+
+ public Binding sampledTrace(SampledTraceBuilder sampledTraceBuilder) {
Review Comment:
*[CanIgnoreReturnValueSuggester](https://errorprone.info/bugpattern/CanIgnoreReturnValueSuggester):* Methods that always 'return this' should be annotated with @CanIgnoreReturnValue
---
```suggestion
@CanIgnoreReturnValue
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=349217659&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=349217659&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349217659&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=349217659&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=349217659&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9857:
URL: https://github.com/apache/skywalking/pull/9857
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #9857: Support `sampleTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008707690
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/trace/SampleTraceSlowRecord.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.core.analysis.manual.trace;
+
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.analysis.Stream;
+import org.apache.skywalking.oap.server.core.analysis.record.Record;
+import org.apache.skywalking.oap.server.core.analysis.topn.TopN;
+import org.apache.skywalking.oap.server.core.analysis.worker.RecordStreamProcessor;
+import org.apache.skywalking.oap.server.core.source.ScopeDeclaration;
+import org.apache.skywalking.oap.server.core.storage.annotation.BanyanDB;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
+import org.apache.skywalking.oap.server.core.storage.type.Convert2Entity;
+import org.apache.skywalking.oap.server.core.storage.type.Convert2Storage;
+import org.apache.skywalking.oap.server.core.storage.type.StorageBuilder;
+
+import static org.apache.skywalking.oap.server.core.source.DefaultScopeDefine.SAMPLE_TRACE_SLOW;
+
+@Setter
+@Getter
+@ScopeDeclaration(id = SAMPLE_TRACE_SLOW, name = "SampleTraceSlowRecord")
+@Stream(name = SampleTraceSlowRecord.INDEX_NAME, scopeId = SAMPLE_TRACE_SLOW, builder = SampleTraceSlowRecord.Builder.class, processor = RecordStreamProcessor.class)
+public class SampleTraceSlowRecord extends Record {
Review Comment:
```suggestion
public class SampledSlowTraceRecord extends Record {
```
Please adjust the class, index name, and document accordingly. **Sampled slow trace record**s match the purpose more.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008785320
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/trace/SampleTraceSlowRecord.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.core.analysis.manual.trace;
+
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.Const;
+import org.apache.skywalking.oap.server.core.analysis.Stream;
+import org.apache.skywalking.oap.server.core.analysis.record.Record;
+import org.apache.skywalking.oap.server.core.analysis.topn.TopN;
+import org.apache.skywalking.oap.server.core.analysis.worker.RecordStreamProcessor;
+import org.apache.skywalking.oap.server.core.source.ScopeDeclaration;
+import org.apache.skywalking.oap.server.core.storage.annotation.BanyanDB;
+import org.apache.skywalking.oap.server.core.storage.annotation.Column;
+import org.apache.skywalking.oap.server.core.storage.type.Convert2Entity;
+import org.apache.skywalking.oap.server.core.storage.type.Convert2Storage;
+import org.apache.skywalking.oap.server.core.storage.type.StorageBuilder;
+
+import static org.apache.skywalking.oap.server.core.source.DefaultScopeDefine.SAMPLE_TRACE_SLOW;
+
+@Setter
+@Getter
+@ScopeDeclaration(id = SAMPLE_TRACE_SLOW, name = "SampleTraceSlowRecord")
+@Stream(name = SampleTraceSlowRecord.INDEX_NAME, scopeId = SAMPLE_TRACE_SLOW, builder = SampleTraceSlowRecord.Builder.class, processor = RecordStreamProcessor.class)
+public class SampleTraceSlowRecord extends Record {
Review Comment:
Updated.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294623533
> `caused_by` could be considered as a metric name, so we could have the slow trace sampled record or others. We could indicate this as a parameter of `sampledTrace(sampleReason)` function in LAL.
Do you mean to fix the reason name in LAL? such as `sampledTrace("slow") {}`? For now, the LAL haven't the pre-load ability to read the method value for creating an index/table when OAP starts.
> A question, the collecting HTTP body would not be reported this way, right? It goes through `SpanAttachedEventReportService` directly, rover just reports trace ID here.
Yes, the log only reporters the trace ID, and reports the HTTP body through `SpanAttachedEventReportService`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on a diff in pull request #9857: Support `sampleTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1007926644
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampleTraceBuilder sampleTraceBuilder() {
+ return (SampleTraceBuilder) getProperty(KEY_SAMPLE_TRACE);
+ }
+
+ public Binding sampleTrace(SampleTraceBuilder sampleTraceBuilder) {
Review Comment:
@sonatype-lift ignoreall
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008788218
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampledTraceBuilder sampledTraceBuilder() {
+ return (SampledTraceBuilder) getProperty(KEY_SAMPLED_TRACE);
+ }
+
+ public Binding sampledTrace(SampledTraceBuilder sampledTraceBuilder) {
Review Comment:
The **ignoreall** command is active on this PR, all the existing Lift issues are ignored.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294499356
But we can't control the table/index size of record, right? When querying the sampling trace, we can't be knowing which data is displayed first. Unless we query a group field list, then try to query each top N with the group name?
Otherwise, we can only specify several cause types and then create the corresponding tables/index.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294525697
https://github.com/apache/skywalking-query-protocol/blob/master/record.graphqls
If you mean Order in the condition, we could consider it as optional? As we could have slow trace or other reason(another metric) to sample?
What do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294578250
`caused_by` could be considered as a metric name, so we could have the slow trace sampled record or others. We could indicate this as a parameter of `sampledTrace(sampleReason)` function in LAL.
A question, the collecting HTTP body would not be reported this way, right? It goes through `SpanAttachedEventReportService` directly, rover just reports trace ID here.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008967470
##########
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SampledTraceBuilder.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.analyzer.provider.trace.parser.listener;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.analysis.manual.trace.SampledSlowTraceRecord;
+import org.apache.skywalking.oap.server.core.analysis.record.Record;
+import org.apache.skywalking.oap.server.core.config.NamingControl;
+import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import org.apache.skywalking.oap.server.core.source.DetectPoint;
+import org.apache.skywalking.oap.server.core.source.ISource;
+import org.apache.skywalking.oap.server.core.source.ProcessRelation;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+
+@RequiredArgsConstructor
+public class SampledTraceBuilder {
+ private final NamingControl namingControl;
+
+ @Setter
+ @Getter
+ private String traceId;
+ @Setter
+ @Getter
+ private String uri;
+ @Setter
+ @Getter
+ private long latency;
+ @Setter
+ @Getter
+ private Reason reason;
+
+ @Setter
+ @Getter
+ private String layer;
+ @Setter
+ @Getter
+ private String serviceName;
+ @Setter
+ @Getter
+ private String serviceInstanceName;
+ @Setter
+ @Getter
+ private String processId;
+ @Setter
+ @Getter
+ private String destProcessId;
+ @Setter
+ @Getter
+ private int componentId;
+ @Setter
+ @Getter
+ private DetectPoint detectPoint;
+
+ @Setter
+ @Getter
+ private long timestamp;
+
+ public String validate() {
+ String result = null;
+ result = validateNotEmpty(result, "traceId", traceId);
Review Comment:
Updated, please help to re-check.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `sampleTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1007909827
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampleTraceBuilder sampleTraceBuilder() {
+ return (SampleTraceBuilder) getProperty(KEY_SAMPLE_TRACE);
+ }
+
+ public Binding sampleTrace(SampleTraceBuilder sampleTraceBuilder) {
Review Comment:
*[CanIgnoreReturnValueSuggester](https://errorprone.info/bugpattern/CanIgnoreReturnValueSuggester):* Methods that always 'return this' should be annotated with @CanIgnoreReturnValue
---
```suggestion
@CanIgnoreReturnValue
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=348962145&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=348962145&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348962145&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348962145&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=348962145&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294389028
:warning: **6 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/skywalking/01GGE6P5FZ92MBDTHWJ1KP2Q0B?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20skywalking) for more details.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294456139
We could build a format of sampled trace for this kind of data yo include reason. More fields seem hard to maintain.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `sampleTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1007926681
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampleTraceBuilder sampleTraceBuilder() {
+ return (SampleTraceBuilder) getProperty(KEY_SAMPLE_TRACE);
+ }
+
+ public Binding sampleTrace(SampleTraceBuilder sampleTraceBuilder) {
Review Comment:
The **ignoreall** command is active on this PR, all the existing Lift issues are ignored.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294430822
> But in ebpf, we may sample all requests with trace headers, rather than only slow, right?
Yes, In the eBPF we can sample all kinds of requests with trace headers, for now, we only have the `slow` cases. There may be more in the future.
> And in the record, it would be SampledTraceRecord, with a column of sampling reason is slow. WDYT?
It's good to be, when the rover agent uploads the logs, they need to add a `reason` field to confirm the sampling reason. But for the GraphQL query, for now, we only could be ordering the record. We need a filter mechanism to filter the `reason` filed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294637200
> > `caused_by` could be considered as a metric name, so we could have the slow trace sampled record or others. We could indicate this as a parameter of `sampledTrace(sampleReason)` function in LAL.
>
> Do you mean to fix the reason name in LAL? such as `sampledTrace("slow") {}`? For now, the LAL haven't the pre-load ability to read the method value for creating an index/table when OAP starts.
I don't have to be that complex. I just mean `slow` could be an enum or string, we just need an IF/ELSE in the LAL kernel.
The point of proposing this is that, we may need to extend later, but we may don't want to keep adding a new LAL method to cause confusing. The record means physical tables in SQL database, and an index in un-merging mode(before 9.2.0, and still supported now). This could not be a light concept, and loaded through LAL. So, don't worry for that.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] sonatype-lift[bot] commented on a diff in pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1007575279
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SlowTraceBuilder slowTraceBuilder() {
+ return (SlowTraceBuilder) getProperty(KEY_SLOW_TRACE);
+ }
+
+ public Binding slowTrace(SlowTraceBuilder slowTraceBuilder) {
Review Comment:
*[CanIgnoreReturnValueSuggester](https://errorprone.info/bugpattern/CanIgnoreReturnValueSuggester):* Methods that always 'return this' should be annotated with @CanIgnoreReturnValue
---
```suggestion
@CanIgnoreReturnValue
```
---
<details><summary><b>âšī¸ Learn about @sonatype-lift commands</b></summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
Was this a good recommendation?
[ [đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=348807501&lift_comment_rating=1) ] - [ [đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=348807501&lift_comment_rating=2) ] - [ [đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348807501&lift_comment_rating=3) ] - [ [đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=348807501&lift_comment_rating=4) ] - [ [đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=348807501&lift_comment_rating=5) ]
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294423008
How about `sampleTrace` rather than `slowTrace`? SlowStatement is a thing we built for SQL database, as we only would collect that.
But in ebpf, we may sample all requests with trace headers, rather than only slow, right?
I prefer we could provide a more general use name.
And in the record, it would be SampledTraceRecord, with a column of sampling reason is `slow`. WDYT?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on pull request #9857: Support `slowTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#issuecomment-1294539340
I think the order field is necessary. The record tables only insert the sampled traces, but maybe not ordering.
Such as we have the following records:
```
{"reason": "slow", "name": "slow-/requestURI", "latency": 90}
{"reason": "slow", "name": "slow-/requestURI", "latency": 80}
{"reason": "slow", "name": "slow-/requestURI", "latency": 100}
{"reason": "error", "name": "error-/requestURI", "latency": 70}
```
Then, If we want to query the sampled records with top2. we may face the following issues:
1. If not ordering, the top <N> records are not correct. Because we just insert the record in the OAP and order the records when query.
2. If ordering, we may only see a part of the data or missing data(in this case, the error reason records are missing). So I recommend we query the reason list first(in this case, it will be "slow" and "error"), then query each top N record with the same reason.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mrproliu commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
mrproliu commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008788205
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/dsl/Binding.java:
##########
@@ -107,6 +110,15 @@ public Binding databaseSlowStatement(DatabaseSlowStatementBuilder databaseSlowSt
return this;
}
+ public SampledTraceBuilder sampledTraceBuilder() {
+ return (SampledTraceBuilder) getProperty(KEY_SAMPLED_TRACE);
+ }
+
+ public Binding sampledTrace(SampledTraceBuilder sampledTraceBuilder) {
Review Comment:
@sonatype-lift ignoreall
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #9857: Support `sampledTrace` in LAL
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9857:
URL: https://github.com/apache/skywalking/pull/9857#discussion_r1008964419
##########
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SampledTraceBuilder.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.analyzer.provider.trace.parser.listener;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.analysis.manual.trace.SampledSlowTraceRecord;
+import org.apache.skywalking.oap.server.core.analysis.record.Record;
+import org.apache.skywalking.oap.server.core.config.NamingControl;
+import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import org.apache.skywalking.oap.server.core.source.DetectPoint;
+import org.apache.skywalking.oap.server.core.source.ISource;
+import org.apache.skywalking.oap.server.core.source.ProcessRelation;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+
+@RequiredArgsConstructor
+public class SampledTraceBuilder {
+ private final NamingControl namingControl;
+
+ @Setter
+ @Getter
+ private String traceId;
+ @Setter
+ @Getter
+ private String uri;
+ @Setter
+ @Getter
+ private long latency;
+ @Setter
+ @Getter
+ private Reason reason;
+
+ @Setter
+ @Getter
+ private String layer;
+ @Setter
+ @Getter
+ private String serviceName;
+ @Setter
+ @Getter
+ private String serviceInstanceName;
+ @Setter
+ @Getter
+ private String processId;
+ @Setter
+ @Getter
+ private String destProcessId;
+ @Setter
+ @Getter
+ private int componentId;
+ @Setter
+ @Getter
+ private DetectPoint detectPoint;
+
+ @Setter
+ @Getter
+ private long timestamp;
+
+ public String validate() {
+ String result = null;
+ result = validateNotEmpty(result, "traceId", traceId);
Review Comment:
Can we just use Guava `Preconditions`?
```java
Preconditions.checkArgument(!Strings.isNullOrEmpty(traceId), "traceId can't be empty");
```
##########
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SampledTraceBuilder.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.oap.server.analyzer.provider.trace.parser.listener;
+
+import lombok.Getter;
+import lombok.RequiredArgsConstructor;
+import lombok.Setter;
+import org.apache.skywalking.oap.server.core.analysis.DownSampling;
+import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.Layer;
+import org.apache.skywalking.oap.server.core.analysis.TimeBucket;
+import org.apache.skywalking.oap.server.core.analysis.manual.trace.SampledSlowTraceRecord;
+import org.apache.skywalking.oap.server.core.analysis.record.Record;
+import org.apache.skywalking.oap.server.core.config.NamingControl;
+import org.apache.skywalking.oap.server.core.source.DefaultScopeDefine;
+import org.apache.skywalking.oap.server.core.source.DetectPoint;
+import org.apache.skywalking.oap.server.core.source.ISource;
+import org.apache.skywalking.oap.server.core.source.ProcessRelation;
+import org.apache.skywalking.oap.server.library.util.StringUtil;
+
+@RequiredArgsConstructor
+public class SampledTraceBuilder {
+ private final NamingControl namingControl;
+
+ @Setter
+ @Getter
+ private String traceId;
+ @Setter
+ @Getter
+ private String uri;
+ @Setter
+ @Getter
+ private long latency;
+ @Setter
+ @Getter
+ private Reason reason;
+
+ @Setter
+ @Getter
+ private String layer;
+ @Setter
+ @Getter
+ private String serviceName;
+ @Setter
+ @Getter
+ private String serviceInstanceName;
+ @Setter
+ @Getter
+ private String processId;
+ @Setter
+ @Getter
+ private String destProcessId;
+ @Setter
+ @Getter
+ private int componentId;
+ @Setter
+ @Getter
+ private DetectPoint detectPoint;
+
+ @Setter
+ @Getter
+ private long timestamp;
+
+ public String validate() {
+ String result = null;
+ result = validateNotEmpty(result, "traceId", traceId);
+ result = validateBiggerZero(result, "latency", latency);
+ result = validateNotEmpty(result, "uri", uri);
+ result = validateNotEmpty(result, "reason", reason);
+ result = validateNotEmpty(result, "layer", layer);
+ result = validateNotEmpty(result, "service name", serviceName);
+ result = validateNotEmpty(result, "service instance name", serviceInstanceName);
+ result = validateNotEmpty(result, "process id", processId);
+ result = validateNotEmpty(result, "dest process id", destProcessId);
+ result = validateNotEmpty(result, "detect point", detectPoint);
+ result = validateNotEmpty(result, "dest process id", destProcessId);
+ result = validateBiggerZero(result, "component id", componentId);
+ result = validateBiggerZero(result, "timestamp", timestamp);
+ return result;
+ }
+
+ public Record toRecord() {
+ final SampledSlowTraceRecord record = new SampledSlowTraceRecord();
+ record.setScope(DefaultScopeDefine.PROCESS_RELATION);
+ record.setEntityId(IDManager.ProcessID.buildRelationId(new IDManager.ProcessID.ProcessRelationDefine(
+ processId, destProcessId
+ )));
+ record.setTraceId(traceId);
+ record.setUri(uri);
+ record.setLatency(latency);
+ record.setTimeBucket(TimeBucket.getTimeBucket(timestamp, DownSampling.Second));
+ return record;
+ }
+
+ public ISource toEntity() {
+ final ProcessRelation processRelation = new ProcessRelation();
+ final String serviceId = IDManager.ServiceID.buildId(namingControl.formatServiceName(serviceName),
+ Layer.nameOf(layer).isNormal());
+ final String instanceId = IDManager.ServiceInstanceID.buildId(serviceId, namingControl.formatInstanceName(serviceInstanceName));
+ processRelation.setInstanceId(instanceId);
+ processRelation.setSourceProcessId(processId);
+ processRelation.setDestProcessId(destProcessId);
+ processRelation.setDetectPoint(detectPoint);
+ processRelation.setComponentId(componentId);
+ return processRelation;
+ }
+
+ private String validateNotEmpty(String error, String name, String message) {
+ if (error != null) {
+ return error;
+ }
+ if (StringUtil.isEmpty(message)) {
+ return name + " not configured.";
+ }
+ return null;
+ }
+
+ private String validateNotEmpty(String error, String name, Object data) {
+ if (error != null) {
+ return error;
+ }
+ if (data == null) {
+ return name + " not configured.";
+ }
+ return null;
+ }
+
+ private String validateBiggerZero(String error, String name, long val) {
+ if (error != null) {
+ return error;
+ }
+ if (val < 0) {
+ return name + " must bigger zero.";
+ }
Review Comment:
This doesn't check `0`, `val == 0` is not `BiggerZero` but no error is reported
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org