[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r519429655 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: > Could you leave a comment that implies the location and commit sha? It is too tricky to maintain the codes without them even though the original contributor like you doesn't figure it out after several months. Syncing is a more complicated topic, as there are a lot of unversioned protocol files in the repo, need to be discussed further. Comment is added 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800:
URL: https://github.com/apache/skywalking/pull/5800#discussion_r518787734
##
File path: .github/workflows/e2e.istio.yaml
##
@@ -73,7 +77,7 @@ jobs:
--set elasticsearch.replicas=1 \
--set elasticsearch.minimumMasterNodes=1 \
--set elasticsearch.imageTag=7.5.1 \
- --set oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=k8s-mesh \
+ --set oap.env.SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=${{
matrix.analyzer }} \
Review comment:
Because `metadataExchange` is enabled when `telemetry.v2` is enabled,
and `telemetry.v2` is enable by default in the `demo` profile, we install Istio
with `profile=demo`, hence `metadataExchange` is enabled too
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518636664 ## File path: docs/en/setup/envoy/als_setting.md ## @@ -19,7 +19,7 @@ You need three steps to open ALS. Note: SkyWalking OAP service is at skywalking namespace, and the port of gRPC service is 11800 2. (Default is ACTIVATED) Activate SkyWalking [envoy receiver](../backend/backend-receivers.md). -3. Active ALS k8s-mesh analysis, set system env variable `SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS`=`k8s-mesh` +3. Active ALS analyzer, there are two available analyzers, `k8s-mesh` and `mx-mesh`, `k8s-mesh` will use the metadata from Kubernetes cluster, `mx-mesh` will use the Envoy metadata exchange mechanism to get the service name, etc., set system env variable `SW_ENVOY_METRIC_ALS_HTTP_ANALYSIS=k8s-mesh` Review comment: > Could you mention mx-mesh dependencies istio `metadata exchange` filter? DONE 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518636457 ## File path: oap-server/server-bootstrap/src/main/resources/metadata-service-mapping.yaml ## @@ -0,0 +1,17 @@ +# 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. + +serviceName: LABELS.app Review comment: DONE 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518636319 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: I'll just keep it as is, unless @hanahmily give a real use case where we need 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800:
URL: https://github.com/apache/skywalking/pull/5800#discussion_r518635604
##
File path:
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/als/mx/package-info.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ *
+ */
+
+/**
+ * {@link org.apache.skywalking.oap.server.receiver.envoy.als.ALSHTTPAnalysis}
implementation
+ * based on the Envoy metadata exchange.
+ */
+
+package org.apache.skywalking.oap.server.receiver.envoy.als.mx;
Review comment:
Thought it was a good practice, but finally realize that we don't
emphasize too much about the JavaDoc, this makes not much sense
##
File path:
oap-server/server-receiver-plugin/envoy-metrics-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/als/mx/package-info.java
##
@@ -0,0 +1,24 @@
+/*
+ * 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.
+ *
+ */
+
+/**
+ * {@link org.apache.skywalking.oap.server.receiver.envoy.als.ALSHTTPAnalysis}
implementation
+ * based on the Envoy metadata exchange.
+ */
+
+package org.apache.skywalking.oap.server.receiver.envoy.als.mx;
Review comment:
Thought it was a good practice, but finally realize that we don't
emphasize too much about the JavaDoc, this makes not much sense, deleted
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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518635223 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. + * + * Licensed 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. + */ Review comment: Can you please kindly recheck? 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518592664 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: If that's the case, I'm afraid we have to clone the repo just for that one file. @hanahmily can you elaborate more about why we need an update mechanism to this file? What makes it different from the other files that we copied from, like Envoy, Jaeger, etc.? 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518520232 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: Yes, I'll configure a maven phase to download this before compiling, with a URL format like this, the commit SHA is a property that can be specified (the necessity needs discussion though) `https://raw.githubusercontent.com/istio/proxy/94a7d9942aa8181f688933cb265879d59a0d85dd/extensions/common/node_info.fbs` 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518516795 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: > What is the size of downloading. I assume this would make compiling more unstable, especially in China. 1.6KB, we only download that single file with a specific commit SHA 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518516795 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: > What is the size of downloading. I assume this would make compiling more unstable, especially in China. 1.6KB 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518514252 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: @wu-sheng 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518509564 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: > and a tool or shell to update it based on this SHA. Doing that means we want to download the file on the fly, do we still need the license for this? @wu-sheng we don't include that in source distribution then, it will be downloaded when compiling. 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
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5800: ALS analyzer based on Envoy metadata exchange
kezhenxu94 commented on a change in pull request #5800: URL: https://github.com/apache/skywalking/pull/5800#discussion_r518509564 ## File path: oap-server/server-receiver-plugin/receiver-proto/src/main/fbs/istio/node-info.fbs ## @@ -0,0 +1,48 @@ +/* Copyright 2020 Istio Authors. All Rights Reserved. Review comment: > and a tool or shell to update it based on this SHA. Doing that means we want to download the file on the fly, do we still need the license for this? @wu-sheng we don't include that in source distribution then. 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
