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 2021/06/25 07:41:49 UTC
[GitHub] [skywalking] zhyyu opened a new issue #7169: dynamic configuration slowDBAccessThreshold work confusing when not configured
zhyyu opened a new issue #7169:
URL: https://github.com/apache/skywalking/issues/7169
Please answer these questions before submitting your issue.
- Why do you submit this issue?
- [x] Question or discussion
- [ ] Bug
- [ ] Requirement
- [ ] Feature or performance improvement
___
### Question
- What do you want to know?
I use dynamic configuration(nacos) to management alarm config, and **not intent** to manage slowDBAccessThreshold. So I do not config the agent-analyzer.default.slowDBAccessThreshold. But it worked weird, the default threshold will be changed to 10000, I read the code and found that if not config slowDBAccessThreshold in nacos, the code will rewrite the default threshold to 10000 because of triggering of DELETE notify. And this will confuse the user, where the 10000 comes from.
```java
DBLatencyThresholdsAndWatcher
private void activeSetting(String config) {
Map<String, Integer> newThresholds = new HashMap<>();
String[] settings = config.split(",");
for (String setting : settings) {
String[] typeValue = setting.split(":");
if (typeValue.length == 2) {
newThresholds.put(typeValue[0].trim().toLowerCase(), Integer.parseInt(typeValue[1].trim()));
}
}
if (!newThresholds.containsKey("default")) {
newThresholds.put("default", 10000);
}
thresholds.set(newThresholds);
settingsString.set(config);
}
public void notify(ConfigChangeEvent value) {
if (EventType.DELETE.equals(value.getEventType())) {
activeSetting("");
} else {
activeSetting(value.getNewValue());
}
}
```
___
### Requirement or improvement
- Please describe your requirements or improvement suggestions.
Can I send an pull request to change the "DELETE" notify behavior?
from
```java
private void activeSetting(String config) {
// ...
if (!newThresholds.containsKey("default")) {
newThresholds.put("default", 10000);
}
// ...
}
```
to (the default setting in application.yml)
```java
private void activeSetting(String config) {
// ...
if (!newThresholds.containsKey("default")) {
newThresholds.put("default", 200);
newThresholds.put("mongodb", 50);
}
// ...
}
```
--
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
[GitHub] [skywalking] gaoweijie commented on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
gaoweijie commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869497380
I also found the same problem, mark and pay attention to the fix result.
--
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 issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869520514
> I am thinking add a dynamicSettingsString to fix it
It should be named `initialSettings`.
> Can I send a pull request?
Yes, please.
--
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] zhyyu commented on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
zhyyu commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869270599
> I want to ask where is the delete event comes from? Why it happens? Did you really delete the key?
I didn't delete the key, I just not config it. And I checked the code, found that
```
ConfigWatcherRegister
void configSync() {
// ...
if (newItemValue == null) {
if (watcher.value() != null) {
// Notify watcher, the new value is null with delete event type.
watcher.notify(
new ConfigChangeWatcher.ConfigChangeEvent(null, ConfigChangeWatcher.EventType.DELETE));
} else {
// Don't need to notify, stay in null.
}
// ...
```
So if we don't config the dynamic config, it will send delete event.
--
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] zhyyu commented on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
zhyyu commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869431894
> If `if (newItemValue == null) {` and `if (watcher.value() == null) {` matched, nothing should happen, like `// Don't need to notify, stay in null.`.
But when init `DBLatencyThresholdsAndWatcher`, it will use default value to set the watcher value, so `watcher.value() == null` cannot be true and then trigger the delete event.
```
public DBLatencyThresholdsAndWatcher(String config, ModuleProvider provider) {
// ...
activeSetting(config);
}
private void activeSetting(String config) {
// ...
settingsString.set(config);
}
@Override
public String value() {
return settingsString.get();
}
```
--
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 issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869423894
If `if (newItemValue == null) {` and `if (watcher.value() == null) {` matched, nothing should happen, like `// Don't need to notify, stay in null.`.
--
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 issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869465210
OK, get your point this time. Then, this is a bug. It should be fixed like this.
If DBLatencyThresholdsAndWatcher never has a notified data, its `#value` method should always return `null`, rather than `settingsString.get()`.
Also, we should keep initial `settingsString` forever, and fall back to it when we received `delete` event. Make sense?
--
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] zhyyu commented on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
zhyyu commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869480880
> settingsString
Yes, it make sense. I am thinking add a `dynamicSettingsString` to fix it. Can I send a pull request?
--
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 issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-868357367
I want to ask where is the delete event comes from? Why it happens? Did you really delete the key?
--
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
[GitHub] [skywalking] zhyyu commented on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
zhyyu commented on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869484799
> OK, get your point this time. Then, this is a bug. It should be fixed like this.
> If DBLatencyThresholdsAndWatcher never has a notified data, its `#value` method should always return `null`, rather than `settingsString.get()`.
> Also, we should keep initial `settingsString` forever, and fall back to it when we received `delete` event. Make sense?
Yes, it make sense. I am thinking add a dynamicSettingsString to fix it. Can I send a pull request?
--
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] zhyyu removed a comment on issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
zhyyu removed a comment on issue #7169:
URL: https://github.com/apache/skywalking/issues/7169#issuecomment-869480880
> settingsString
Yes, it make sense. I am thinking add a `dynamicSettingsString` to fix it. Can I send a pull request?
--
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 closed issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #7169:
URL: https://github.com/apache/skywalking/issues/7169
--
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 closed issue #7169: dynamic configuration slowDBAccessThreshold rewrite confusing when not configured
Posted by GitBox <gi...@apache.org>.
wu-sheng closed issue #7169:
URL: https://github.com/apache/skywalking/issues/7169
--
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