[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #4802: Group Parameterized Endpoints

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

kezhenxu94 commented on a change in pull request #4802:
URL: https://github.com/apache/skywalking/pull/4802#discussion_r427945387



##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/config/group/EndpointGroupingRule.java
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.config.group;
+
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.skywalking.apm.util.StringFormatGroup;
+
+/**
+ * Endpoint group rule hosts all group rules of all services.
+ */
+public class EndpointGroupingRule {
+    private Map<String, StringFormatGroup> rules = new HashMap<>();
+
+    /**
+     * Add a new rule to the context.
+     *
+     * @param serviceName       of the new rule
+     * @param endpointGroupName represents the logic endpoint name.
+     * @param ruleRegex         match the endpoints which should be in the group name.
+     */
+    void addRule(String serviceName, String endpointGroupName, String ruleRegex) {
+        if (rules.containsKey(serviceName)) {
+            rules.get(serviceName).addRule(endpointGroupName, ruleRegex);
+        } else {
+            StringFormatGroup newGroup = new StringFormatGroup();
+            newGroup.addRule(endpointGroupName, ruleRegex);
+            rules.put(serviceName, newGroup);
+        }

Review comment:
       -> `computeIfAbsent`

##########
File path: docs/en/setup/backend/endpoint-grouping-rules.md
##########
@@ -0,0 +1,23 @@
+# Group Parameterized Endpoints
+In most cases, the endpoint should be detected automatically through the language agents, service mesh observability solution, 
+or configuration of meter system.
+
+There are some special cases, especially when people use REST style URI, the application codes put the parameter in the endpoint name, 
+such as putting order id in the URI, like `/prod/ORDER123` and `/prod/ORDER123`. But logically, people expect they could
+have an endpoint name like `prod/{order-id}`. This is the feature of parameterized endpoint grouping designed for.
+
+Current, user could set up grouping rules through the static YAML file, named `endpoint_name_grouping.yml`,
+or use [Dynamic Configuration](dynamic-config.md) to initial and update the endpoint grouping rule.
+
+## Configuration Format
+No matter in static local file or dynamic configuration value, they are sharing the same YAML format.
+
+```yaml
+grouping:
+  # Endpoint of the service would follow the following rules
+  - service-name: serviceA

Review comment:
       I'm wondering whether it's possible not to configure `service-name`, we have cases where different services share the same endpoint name, rules without service name may be a great help to save time

##########
File path: oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/config/group/EndpointNameGroupingRuleWatcherTest.java
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.config.group;
+
+import java.io.FileNotFoundException;
+import org.apache.skywalking.oap.server.configuration.api.ConfigChangeWatcher;
+import org.apache.skywalking.oap.server.core.CoreModule;
+import org.apache.skywalking.oap.server.library.module.ModuleConfig;
+import org.apache.skywalking.oap.server.library.module.ModuleDefine;
+import org.apache.skywalking.oap.server.library.module.ModuleProvider;
+import org.apache.skywalking.oap.server.library.module.ModuleStartException;
+import org.apache.skywalking.oap.server.library.module.ServiceNotProvidedException;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class EndpointNameGroupingRuleWatcherTest {
+    @Test
+    public void testWatcher() throws FileNotFoundException {
+        EndpointNameGrouping endpointNameGrouping = new EndpointNameGrouping();
+
+        EndpointNameGroupingRuleWatcher watcher = new EndpointNameGroupingRuleWatcher(
+            new ModuleProvider() {
+                @Override
+                public String name() {
+                    return "test";
+                }
+
+                @Override
+                public Class<? extends ModuleDefine> module() {
+                    return CoreModule.class;
+                }
+
+                @Override
+                public ModuleConfig createConfigBeanIfAbsent() {
+                    return null;
+                }
+
+                @Override
+                public void prepare() throws ServiceNotProvidedException, ModuleStartException {
+
+                }
+
+                @Override
+                public void start() throws ServiceNotProvidedException, ModuleStartException {
+
+                }
+
+                @Override
+                public void notifyAfterCompleted() throws ServiceNotProvidedException, ModuleStartException {
+
+                }
+
+                @Override
+                public String[] requiredModules() {
+                    return new String[0];
+                }
+            }, endpointNameGrouping);
+        Assert.assertEquals("/prod/{id}", endpointNameGrouping.format("serviceA", "/prod/123"));
+
+        watcher.notify(new ConfigChangeWatcher.ConfigChangeEvent(
+            "# Licensed to the Apache Software Foundation (ASF) under one or more\n" +
+                "# contributor license agreements.  See the NOTICE file distributed with\n" +
+                "# this work for additional information regarding copyright ownership.\n" +
+                "# The ASF licenses this file to You under the Apache License, Version 2.0\n" +
+                "# (the \"License\"); you may not use this file except in compliance with\n" +
+                "# the License.  You may obtain a copy of the License at\n" +
+                "#\n" +
+                "#     http://www.apache.org/licenses/LICENSE-2.0\n" +
+                "#\n" +
+                "# Unless required by applicable law or agreed to in writing, software\n" +
+                "# distributed under the License is distributed on an \"AS IS\" BASIS,\n" +
+                "# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.\n" +
+                "# See the License for the specific language governing permissions and\n" +
+                "# limitations under the License.\n" +
+                "\n" +
+                "# Endpoint name grouping rules.\n" +
+                "# In most cased, endpoint name should be detected by agents or service mesh automatically, and aggregate the metrics based\n" +
+                "# on the name.\n" +
+                "# But, in some cases, application put the parameter in the endpoint name, such as putting order id in the URI, like\n" +
+                "# /prod/ORDER123, /prod/ORDER123.\n" +
+                "# This grouping file provides the regex based definition capability to merge those endpoints into a group by better and\n" +
+                "# more meaningful aggregation metrics.\n" +
+                "\n" +
+                "grouping:\n" +
+                "  # Endpoint of the service would follow the following rules\n" +
+                "  - service-name: serviceA\n" +
+                "    rules:\n" +
+                "      - endpoint-name: /prod/order-id\n" +
+                "        regex: \\/prod\\/.+"

Review comment:
       Well, if it's inlined in the test codes, why not just simplify them, i.e. keep only uncommented content




----------------------------------------------------------------
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