[GitHub] [skywalking] wu-sheng commented on a change in pull request #4228: Support Browser protocol at OAP

[GitBox <gi...@apache.org>]

wu-sheng commented on a change in pull request #4228:
URL: https://github.com/apache/skywalking/pull/4228#discussion_r472889616



##########
File path: docs/README.md
##########
@@ -91,6 +91,7 @@ If you are already familiar with SkyWalking, you could use this catalog to find
       * [Uninstrumented Gateways](en/setup/backend/uninstrumented-gateways.md). Configure gateways/proxies that are not supported by SkyWalking agent plugins, to reflect the delegation in topology graph.
     * [UI setup document](en/setup/backend/ui-setup.md).
     * [CLI setup document](https://github.com/apache/skywalking-cli).
+    * [Browser performance monitoring setup document](https://github.com/apache/skywalking-client-js).

Review comment:
       I think this should be linked as a Browser agent, not here.

##########
File path: README.md
##########
@@ -28,7 +28,7 @@ The core features are following.
 - Distributed tracing and context propagation
 - Database access metrics. Detect slow database access statements(including SQL statements).
 - Alarm
-
+- [WIP] Browser performance monitoring

Review comment:
       Forget to change.

##########
File path: oap-server/server-bootstrap/src/main/resources/oal/browser.oal
##########
@@ -0,0 +1,73 @@
+/*
+* 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.
+*
+*/
+// browser app

Review comment:
       Do we really need so many metrics? Please recheck @arugal @Fine0830. From the UI prototype, it doesn't need so much. Please analysis the metrics used by UI.

##########
File path: docs/en/concepts-and-designs/oal.md
##########
@@ -78,10 +78,15 @@ The parameter (2) is the status of this request. The status(success/failure) eff
 **percentile** is the first multiple value metrics, introduced since 7.0.0. As having multiple values, it could be query through `getMultipleLinearIntValues` GraphQL query.
 In this case, `p99`, `p95`, `p90`, `p75`, `p50` of all incoming request. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 Before 7.0.0, use `p99`, `p95`, `p90`, `p75`, `p50` func(s) to calculate metrics separately. Still supported in 7.x, but don't be recommended, and don't be included in official OAL script. 
-> All_p99 = from(All.latency).p99(10);
+> all_p99 = from(All.latency).p99(10);
 
 In this case, p99 value of all incoming requests. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 
+- `errorRate`. The error rate of browser traffic (only browser).

Review comment:
       If the percent is not suitable, then you should consider whether the scope design is correct.

##########
File path: docs/en/concepts-and-designs/oal.md
##########
@@ -78,10 +78,15 @@ The parameter (2) is the status of this request. The status(success/failure) eff
 **percentile** is the first multiple value metrics, introduced since 7.0.0. As having multiple values, it could be query through `getMultipleLinearIntValues` GraphQL query.
 In this case, `p99`, `p95`, `p90`, `p75`, `p50` of all incoming request. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 Before 7.0.0, use `p99`, `p95`, `p90`, `p75`, `p50` func(s) to calculate metrics separately. Still supported in 7.x, but don't be recommended, and don't be included in official OAL script. 
-> All_p99 = from(All.latency).p99(10);
+> all_p99 = from(All.latency).p99(10);
 
 In this case, p99 value of all incoming requests. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 
+- `errorRate`. The error rate of browser traffic (only browser).

Review comment:
       `percent` is for this. I don't think we need this.

##########
File path: docs/en/protocols/README.md
##########
@@ -29,6 +29,12 @@ SkyWalking javaagent begins to support this since 8.0.0.
 
 [SkyWalking Trace Data Protocol v3](Trace-Data-Protocol-v3.md) defines the communication way and format between agent and backend.
 
+### Browser probe protocol
+
+The probe in [skywalking-client-js](https://github.com/apache/skywalking-client-js) cloud use this protocol to send to backendEnd. This service provided by gRPC.

Review comment:
       ```suggestion
   The browser probe, such as  [skywalking-client-js](https://github.com/apache/skywalking-client-js) could use this protocol to send to backend. This service provided by gRPC.
   ```

##########
File path: docs/en/concepts-and-designs/oal.md
##########
@@ -78,10 +78,15 @@ The parameter (2) is the status of this request. The status(success/failure) eff
 **percentile** is the first multiple value metrics, introduced since 7.0.0. As having multiple values, it could be query through `getMultipleLinearIntValues` GraphQL query.
 In this case, `p99`, `p95`, `p90`, `p75`, `p50` of all incoming request. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 Before 7.0.0, use `p99`, `p95`, `p90`, `p75`, `p50` func(s) to calculate metrics separately. Still supported in 7.x, but don't be recommended, and don't be included in official OAL script. 
-> All_p99 = from(All.latency).p99(10);
+> all_p99 = from(All.latency).p99(10);
 
 In this case, p99 value of all incoming requests. The parameter is the precision of p99 latency calculation, such as in above case, 120ms and 124 are considered same.
 
+- `errorRate`. The error rate of browser traffic (only browser).

Review comment:
       And, I can't find this in the real OAL script. please 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:
users@infra.apache.org