Allow querying logs only by instance to simplify frontend query#9157
Allow querying logs only by instance to simplify frontend query#9157
Conversation
kezhenxu94
added
backend
|
|
||
| protected Map<String, String> ensureInstanceHasPodProperties(final ServiceInstance instance) { | ||
| if (instance == null) { | ||
| throw new IllegalArgumentException("No such instance"); |
There was a problem hiding this comment.
Should we return an empty container list? rather than an exception?
There was a problem hiding this 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"
There was a problem hiding this 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.
|
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. |
Why? UI already displays the errors reported by backend correctly, |
|
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.
|
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
kezhenxu94
force-pushed
the
ondemandlog2
branch
3 times, most recently
from
8028042 to
838ba93
Compare
| return attributesMap; | ||
| } | ||
|
|
||
| public List<String> listContainers( |
There was a problem hiding this 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.
|
Notice, please fix Also, could you reword this doc to follow the latest changes? Please highlight those two key properties, and which values should be put. |
2 participants
CHANGESlog.