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/06/01 07:33:34 UTC
[GitHub] [skywalking] kezhenxu94 opened a new pull request, #9157: Allow querying logs only by instance to simplify frontend query
kezhenxu94 opened a new pull request, #9157:
URL: https://github.com/apache/skywalking/pull/9157
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
<!-- ==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👇 ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== 🆕 Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist 👆 ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/changelog/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] kezhenxu94 commented on a diff in pull request #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886589484
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/instance/InstanceTraffic.java:
##########
@@ -175,6 +175,8 @@ public Metrics toDay() {
}
public static class PropertyUtil {
+ public static final String NAMESPACE = "namespace";
+ public static final String POD = "pod";
Review Comment:
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.
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#issuecomment-1143313278
Notice, please fix `https://skywalking.apache.org/docs/main/latest/en/setup/backend/on-demand-pod-log/`'s menu, there is a extra `g` in the menu link.
Also, could you reword this doc to follow the latest changes? Please highlight those two key properties, and which values should be put.
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9157:
URL: https://github.com/apache/skywalking/pull/9157
--
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 pull request #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#issuecomment-1143248496
> If you want this available on the UI, we could add one more field for the response.
Why? UI already displays the errors reported by backend correctly,
<img width="809" alt="Screen Shot 2022-06-01 at 15 59 26" src="https://user-images.githubusercontent.com/15965696/171356487-398bbe9e-1e43-4895-b3b7-b2522ed18955.png">
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886411675
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/resolver/OndemandLogQuery.java:
##########
@@ -34,64 +33,86 @@
import com.google.gson.Gson;
import org.apache.skywalking.oap.query.graphql.type.InternalLog;
import org.apache.skywalking.oap.query.graphql.type.LogAdapter;
+import org.apache.skywalking.oap.query.graphql.type.OndemandContainergQueryCondition;
import org.apache.skywalking.oap.query.graphql.type.OndemandLogQueryCondition;
-import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.manual.instance.InstanceTraffic.PropertyUtil;
import org.apache.skywalking.oap.server.core.query.input.Duration;
+import org.apache.skywalking.oap.server.core.query.type.Attribute;
import org.apache.skywalking.oap.server.core.query.type.Log;
import org.apache.skywalking.oap.server.core.query.type.Logs;
+import org.apache.skywalking.oap.server.core.query.type.ServiceInstance;
import org.apache.skywalking.oap.server.library.util.StringUtil;
import graphql.kickstart.tools.GraphQLQueryResolver;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1Container;
-import io.kubernetes.client.openapi.models.V1Namespace;
-import io.kubernetes.client.openapi.models.V1NamespaceList;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodSpec;
import io.kubernetes.client.util.Config;
+import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@Slf4j
+@RequiredArgsConstructor
public class OndemandLogQuery implements GraphQLQueryResolver {
private final Gson gson = new Gson();
private final Type responseType = new TypeToken<Map<String, Object>>() {
}.getType();
private CoreV1Api kApi;
- public List<String> listNamespaces() throws IOException {
- try {
- final V1NamespaceList nsList =
- kApi().listNamespace(null, null, null, null, null, null, null, null, null, null);
- return nsList
- .getItems()
- .stream()
- .map(V1Namespace::getMetadata)
- .filter(Objects::nonNull)
- .map(V1ObjectMeta::getName)
- .collect(Collectors.toList());
- } catch (ApiException e) {
- log.error("Failed to list namespaces from Kubernetes, {}", e.getResponseBody(), e);
+ private final MetadataQueryV2 metadataQuery;
- Map<String, Object> responseBody = gson.fromJson(e.getResponseBody(), responseType);
- String message = responseBody.getOrDefault("message", e.getCode()).toString();
- throw new RuntimeException(message);
- }
+ public List<String> listContainers(final OndemandContainergQueryCondition condition)
+ throws IOException {
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return listContainers(ns, pod);
}
- public List<String> listContainers(final OndemandLogQueryCondition condition)
+ public Logs ondemandPodLogs(final OndemandLogQueryCondition condition)
throws IOException {
- final String ns = condition.getNamespace();
- final IDManager.ServiceInstanceID.InstanceIDDefinition instanceIDDefinition =
- IDManager.ServiceInstanceID.analysisId(condition.getServiceInstanceId());
- final String instanceName = instanceIDDefinition.getName();
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return ondemandPodLogs(ns, pod, condition);
+ }
+ protected Map<String, String> ensureInstanceHasPodProperties(final ServiceInstance instance) {
+ if (instance == null) {
+ throw new IllegalArgumentException("No such instance");
Review Comment:
> Should we return an empty container list? rather than an exception?
An exception is more reasonable in my opinion for non-existing instance, I think "no such instance" is totally different from "no logs in this instance"
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#issuecomment-1143253598
This kind of exception would make us suffer many questions from new users since the day we release this, from my experience. Many of them are throwing any exception to this issue channel without thinking or reading.
`The on-demand log is not available for this instance.` may be a better statement on the on-demand widget.
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886549307
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/instance/InstanceTraffic.java:
##########
@@ -175,6 +175,8 @@ public Metrics toDay() {
}
public static class PropertyUtil {
+ public static final String NAMESPACE = "namespace";
+ public static final String POD = "pod";
Review Comment:
Could you add comments for these two to show these are key properties for on-demand logs? And `@since`?
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#issuecomment-1143230605
I think generally, we should not report errors if this instance doesn't have on-demand logs.
If you want this available on the UI, we could add one more field for the response.
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886587640
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/instance/InstanceTraffic.java:
##########
@@ -175,6 +175,8 @@ public Metrics toDay() {
}
public static class PropertyUtil {
+ public static final String NAMESPACE = "namespace";
+ public static final String POD = "pod";
Review Comment:
I seems missing these two.
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/manual/instance/InstanceTraffic.java:
##########
@@ -175,6 +175,8 @@ public Metrics toDay() {
}
public static class PropertyUtil {
+ public static final String NAMESPACE = "namespace";
+ public static final String POD = "pod";
Review Comment:
It seems missing these two.
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886409739
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/resolver/OndemandLogQuery.java:
##########
@@ -34,64 +33,86 @@
import com.google.gson.Gson;
import org.apache.skywalking.oap.query.graphql.type.InternalLog;
import org.apache.skywalking.oap.query.graphql.type.LogAdapter;
+import org.apache.skywalking.oap.query.graphql.type.OndemandContainergQueryCondition;
import org.apache.skywalking.oap.query.graphql.type.OndemandLogQueryCondition;
-import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.manual.instance.InstanceTraffic.PropertyUtil;
import org.apache.skywalking.oap.server.core.query.input.Duration;
+import org.apache.skywalking.oap.server.core.query.type.Attribute;
import org.apache.skywalking.oap.server.core.query.type.Log;
import org.apache.skywalking.oap.server.core.query.type.Logs;
+import org.apache.skywalking.oap.server.core.query.type.ServiceInstance;
import org.apache.skywalking.oap.server.library.util.StringUtil;
import graphql.kickstart.tools.GraphQLQueryResolver;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1Container;
-import io.kubernetes.client.openapi.models.V1Namespace;
-import io.kubernetes.client.openapi.models.V1NamespaceList;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodSpec;
import io.kubernetes.client.util.Config;
+import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@Slf4j
+@RequiredArgsConstructor
public class OndemandLogQuery implements GraphQLQueryResolver {
private final Gson gson = new Gson();
private final Type responseType = new TypeToken<Map<String, Object>>() {
}.getType();
private CoreV1Api kApi;
- public List<String> listNamespaces() throws IOException {
- try {
- final V1NamespaceList nsList =
- kApi().listNamespace(null, null, null, null, null, null, null, null, null, null);
- return nsList
- .getItems()
- .stream()
- .map(V1Namespace::getMetadata)
- .filter(Objects::nonNull)
- .map(V1ObjectMeta::getName)
- .collect(Collectors.toList());
- } catch (ApiException e) {
- log.error("Failed to list namespaces from Kubernetes, {}", e.getResponseBody(), e);
+ private final MetadataQueryV2 metadataQuery;
- Map<String, Object> responseBody = gson.fromJson(e.getResponseBody(), responseType);
- String message = responseBody.getOrDefault("message", e.getCode()).toString();
- throw new RuntimeException(message);
- }
+ public List<String> listContainers(final OndemandContainergQueryCondition condition)
+ throws IOException {
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return listContainers(ns, pod);
}
- public List<String> listContainers(final OndemandLogQueryCondition condition)
+ public Logs ondemandPodLogs(final OndemandLogQueryCondition condition)
throws IOException {
- final String ns = condition.getNamespace();
- final IDManager.ServiceInstanceID.InstanceIDDefinition instanceIDDefinition =
- IDManager.ServiceInstanceID.analysisId(condition.getServiceInstanceId());
- final String instanceName = instanceIDDefinition.getName();
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return ondemandPodLogs(ns, pod, condition);
+ }
+ protected Map<String, String> ensureInstanceHasPodProperties(final ServiceInstance instance) {
+ if (instance == null) {
+ throw new IllegalArgumentException("No such instance");
Review Comment:
Should we return an empty container list? rather than an exception?
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886484582
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/resolver/OndemandLogQuery.java:
##########
@@ -34,64 +33,86 @@
import com.google.gson.Gson;
import org.apache.skywalking.oap.query.graphql.type.InternalLog;
import org.apache.skywalking.oap.query.graphql.type.LogAdapter;
+import org.apache.skywalking.oap.query.graphql.type.OndemandContainergQueryCondition;
import org.apache.skywalking.oap.query.graphql.type.OndemandLogQueryCondition;
-import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.manual.instance.InstanceTraffic.PropertyUtil;
import org.apache.skywalking.oap.server.core.query.input.Duration;
+import org.apache.skywalking.oap.server.core.query.type.Attribute;
import org.apache.skywalking.oap.server.core.query.type.Log;
import org.apache.skywalking.oap.server.core.query.type.Logs;
+import org.apache.skywalking.oap.server.core.query.type.ServiceInstance;
import org.apache.skywalking.oap.server.library.util.StringUtil;
import graphql.kickstart.tools.GraphQLQueryResolver;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1Container;
-import io.kubernetes.client.openapi.models.V1Namespace;
-import io.kubernetes.client.openapi.models.V1NamespaceList;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodSpec;
import io.kubernetes.client.util.Config;
+import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@Slf4j
+@RequiredArgsConstructor
public class OndemandLogQuery implements GraphQLQueryResolver {
private final Gson gson = new Gson();
private final Type responseType = new TypeToken<Map<String, Object>>() {
}.getType();
private CoreV1Api kApi;
- public List<String> listNamespaces() throws IOException {
- try {
- final V1NamespaceList nsList =
- kApi().listNamespace(null, null, null, null, null, null, null, null, null, null);
- return nsList
- .getItems()
- .stream()
- .map(V1Namespace::getMetadata)
- .filter(Objects::nonNull)
- .map(V1ObjectMeta::getName)
- .collect(Collectors.toList());
- } catch (ApiException e) {
- log.error("Failed to list namespaces from Kubernetes, {}", e.getResponseBody(), e);
+ private final MetadataQueryV2 metadataQuery;
- Map<String, Object> responseBody = gson.fromJson(e.getResponseBody(), responseType);
- String message = responseBody.getOrDefault("message", e.getCode()).toString();
- throw new RuntimeException(message);
- }
+ public List<String> listContainers(final OndemandContainergQueryCondition condition)
+ throws IOException {
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return listContainers(ns, pod);
}
- public List<String> listContainers(final OndemandLogQueryCondition condition)
+ public Logs ondemandPodLogs(final OndemandLogQueryCondition condition)
throws IOException {
- final String ns = condition.getNamespace();
- final IDManager.ServiceInstanceID.InstanceIDDefinition instanceIDDefinition =
- IDManager.ServiceInstanceID.analysisId(condition.getServiceInstanceId());
- final String instanceName = instanceIDDefinition.getName();
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return ondemandPodLogs(ns, pod, condition);
+ }
+ protected Map<String, String> ensureInstanceHasPodProperties(final ServiceInstance instance) {
+ if (instance == null) {
+ throw new IllegalArgumentException("No such instance");
+ }
+ final List<Attribute> attributes = instance.getAttributes();
+ if (attributes == null || attributes.isEmpty()) {
+ throw new IllegalArgumentException(
+ "Service instance has no valid properties to locate the Pod");
+ }
+ final Map<String, String> attributesMap =
+ attributes
+ .stream()
+ .collect(Collectors.toMap(Attribute::getName, Attribute::getValue));
+ if (!attributesMap.containsKey(PropertyUtil.NAMESPACE)
+ || !attributesMap.containsKey(PropertyUtil.POD)) {
+ throw new IllegalArgumentException(
+ "Service instance has no valid properties to locate the Pod");
+ }
+ return attributesMap;
+ }
+
+ protected List<String> listContainers(
+ final String namespace,
+ final String podName) throws IOException {
try {
- final V1Pod pod = kApi().readNamespacedPod(instanceName, ns, null);
+ final V1Pod pod = kApi().readNamespacedPod(podName, namespace, null);
final V1PodSpec spec = pod.getSpec();
if (isNull(spec)) {
- throw new RuntimeException(String.format("No spec: %s:%s", ns, instanceName));
+ throw new RuntimeException(String.format("No spec: %s:%s", namespace, podName));
}
final List<String> containers = spec.getContainers().stream()
Review Comment:
*NULL_DEREFERENCE:* object `spec` last assigned on line 113 could be null and is dereferenced at line 118.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
---
Was this a good recommendation?
[ [🙁 Not relevant](https://www.sonatype.com/lift-comment-rating?comment=257597961&lift_comment_rating=1) ] - [ [😕 Won't fix](https://www.sonatype.com/lift-comment-rating?comment=257597961&lift_comment_rating=2) ] - [ [😑 Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=257597961&lift_comment_rating=3) ] - [ [🙂 Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=257597961&lift_comment_rating=4) ] - [ [😊 Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=257597961&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 a diff in pull request #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886547857
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/resolver/OndemandLogQuery.java:
##########
@@ -34,64 +34,87 @@
import com.google.gson.Gson;
import org.apache.skywalking.oap.query.graphql.type.InternalLog;
import org.apache.skywalking.oap.query.graphql.type.LogAdapter;
+import org.apache.skywalking.oap.query.graphql.type.OndemandContainergQueryCondition;
import org.apache.skywalking.oap.query.graphql.type.OndemandLogQueryCondition;
-import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.manual.instance.InstanceTraffic.PropertyUtil;
import org.apache.skywalking.oap.server.core.query.input.Duration;
+import org.apache.skywalking.oap.server.core.query.type.Attribute;
import org.apache.skywalking.oap.server.core.query.type.Log;
import org.apache.skywalking.oap.server.core.query.type.Logs;
+import org.apache.skywalking.oap.server.core.query.type.ServiceInstance;
import org.apache.skywalking.oap.server.library.util.StringUtil;
import graphql.kickstart.tools.GraphQLQueryResolver;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1Container;
-import io.kubernetes.client.openapi.models.V1Namespace;
-import io.kubernetes.client.openapi.models.V1NamespaceList;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodSpec;
import io.kubernetes.client.util.Config;
+import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@Slf4j
+@RequiredArgsConstructor
public class OndemandLogQuery implements GraphQLQueryResolver {
private final Gson gson = new Gson();
private final Type responseType = new TypeToken<Map<String, Object>>() {
}.getType();
private CoreV1Api kApi;
- public List<String> listNamespaces() throws IOException {
- try {
- final V1NamespaceList nsList =
- kApi().listNamespace(null, null, null, null, null, null, null, null, null, null);
- return nsList
- .getItems()
- .stream()
- .map(V1Namespace::getMetadata)
- .filter(Objects::nonNull)
- .map(V1ObjectMeta::getName)
- .collect(Collectors.toList());
- } catch (ApiException e) {
- log.error("Failed to list namespaces from Kubernetes, {}", e.getResponseBody(), e);
+ private final MetadataQueryV2 metadataQuery;
- Map<String, Object> responseBody = gson.fromJson(e.getResponseBody(), responseType);
- String message = responseBody.getOrDefault("message", e.getCode()).toString();
- throw new RuntimeException(message);
- }
+ public List<String> listContainers(final OndemandContainergQueryCondition condition)
+ throws IOException {
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = convertInstancePropertiesToMap(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return listContainers(ns, pod);
}
- public List<String> listContainers(final OndemandLogQueryCondition condition)
+ public Logs ondemandPodLogs(final OndemandLogQueryCondition condition)
throws IOException {
- final String ns = condition.getNamespace();
- final IDManager.ServiceInstanceID.InstanceIDDefinition instanceIDDefinition =
- IDManager.ServiceInstanceID.analysisId(condition.getServiceInstanceId());
- final String instanceName = instanceIDDefinition.getName();
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = convertInstancePropertiesToMap(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return ondemandPodLogs(ns, pod, condition);
+ }
+ protected Map<String, String> convertInstancePropertiesToMap(final ServiceInstance instance) {
+ if (instance == null) {
+ return Collections.emptyMap();
+ }
+ final List<Attribute> attributes = instance.getAttributes();
+ if (attributes == null || attributes.isEmpty()) {
+ return Collections.emptyMap();
+ }
+ final Map<String, String> attributesMap =
+ attributes
+ .stream()
+ .collect(Collectors.toMap(Attribute::getName, Attribute::getValue));
+ if (!attributesMap.containsKey(PropertyUtil.NAMESPACE)
+ || !attributesMap.containsKey(PropertyUtil.POD)) {
+ return Collections.emptyMap();
+ }
+ return attributesMap;
+ }
+
+ public List<String> listContainers(
Review Comment:
@Fine0830 Once UI faces `listContainers` returning an empty list, please show a tip on the logs page, **The on-demand logs are not available.**
--
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 #9157: Allow querying logs only by instance to simplify frontend query
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9157:
URL: https://github.com/apache/skywalking/pull/9157#discussion_r886533557
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/resolver/OndemandLogQuery.java:
##########
@@ -34,64 +33,86 @@
import com.google.gson.Gson;
import org.apache.skywalking.oap.query.graphql.type.InternalLog;
import org.apache.skywalking.oap.query.graphql.type.LogAdapter;
+import org.apache.skywalking.oap.query.graphql.type.OndemandContainergQueryCondition;
import org.apache.skywalking.oap.query.graphql.type.OndemandLogQueryCondition;
-import org.apache.skywalking.oap.server.core.analysis.IDManager;
+import org.apache.skywalking.oap.server.core.analysis.manual.instance.InstanceTraffic.PropertyUtil;
import org.apache.skywalking.oap.server.core.query.input.Duration;
+import org.apache.skywalking.oap.server.core.query.type.Attribute;
import org.apache.skywalking.oap.server.core.query.type.Log;
import org.apache.skywalking.oap.server.core.query.type.Logs;
+import org.apache.skywalking.oap.server.core.query.type.ServiceInstance;
import org.apache.skywalking.oap.server.library.util.StringUtil;
import graphql.kickstart.tools.GraphQLQueryResolver;
import io.kubernetes.client.openapi.ApiException;
import io.kubernetes.client.openapi.Configuration;
import io.kubernetes.client.openapi.apis.CoreV1Api;
import io.kubernetes.client.openapi.models.V1Container;
-import io.kubernetes.client.openapi.models.V1Namespace;
-import io.kubernetes.client.openapi.models.V1NamespaceList;
import io.kubernetes.client.openapi.models.V1ObjectMeta;
import io.kubernetes.client.openapi.models.V1Pod;
import io.kubernetes.client.openapi.models.V1PodSpec;
import io.kubernetes.client.util.Config;
+import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
@Slf4j
+@RequiredArgsConstructor
public class OndemandLogQuery implements GraphQLQueryResolver {
private final Gson gson = new Gson();
private final Type responseType = new TypeToken<Map<String, Object>>() {
}.getType();
private CoreV1Api kApi;
- public List<String> listNamespaces() throws IOException {
- try {
- final V1NamespaceList nsList =
- kApi().listNamespace(null, null, null, null, null, null, null, null, null, null);
- return nsList
- .getItems()
- .stream()
- .map(V1Namespace::getMetadata)
- .filter(Objects::nonNull)
- .map(V1ObjectMeta::getName)
- .collect(Collectors.toList());
- } catch (ApiException e) {
- log.error("Failed to list namespaces from Kubernetes, {}", e.getResponseBody(), e);
+ private final MetadataQueryV2 metadataQuery;
- Map<String, Object> responseBody = gson.fromJson(e.getResponseBody(), responseType);
- String message = responseBody.getOrDefault("message", e.getCode()).toString();
- throw new RuntimeException(message);
- }
+ public List<String> listContainers(final OndemandContainergQueryCondition condition)
+ throws IOException {
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return listContainers(ns, pod);
}
- public List<String> listContainers(final OndemandLogQueryCondition condition)
+ public Logs ondemandPodLogs(final OndemandLogQueryCondition condition)
throws IOException {
- final String ns = condition.getNamespace();
- final IDManager.ServiceInstanceID.InstanceIDDefinition instanceIDDefinition =
- IDManager.ServiceInstanceID.analysisId(condition.getServiceInstanceId());
- final String instanceName = instanceIDDefinition.getName();
+ final ServiceInstance instance =
+ metadataQuery.getInstance(condition.getServiceInstanceId());
+ final Map<String, String> attributesMap = ensureInstanceHasPodProperties(instance);
+ final String ns = attributesMap.get(PropertyUtil.NAMESPACE);
+ final String pod = attributesMap.get(PropertyUtil.POD);
+ return ondemandPodLogs(ns, pod, condition);
+ }
+ protected Map<String, String> ensureInstanceHasPodProperties(final ServiceInstance instance) {
+ if (instance == null) {
+ throw new IllegalArgumentException("No such instance");
Review Comment:
I don't mean for text. I want the UI could visualize this message in a different way.
In the metrics query, you would have seen similar cases that happened before. Before the current v1 and v2 metric query, SkyWalking reports errors when a service/instance doesn't exist, but the client/UI applies a query for that. Back then, we faced a lot of meaningless bug reports. This was why I added a metric mock logic(default value or zero) for the no existing metrics.
I agree that `An exception is more reasonable in my opinion for non-existing instance`. But meanwhile, what the reality told me is that ppl are panic when they saw a red exception.
--
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