You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "brbzull0 (via GitHub)" <gi...@apache.org> on 2024/07/17 09:39:15 UTC
[PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
brbzull0 opened a new pull request, #11560:
URL: https://github.com/apache/trafficserver/pull/11560
(no comment)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#issuecomment-2256309219
Cherry-picked to v10.0.x
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1693376782
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
Ah I see, thank you for explaining!
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1686563739
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,7 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ Warning("%s", swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
Review Comment:
It will go to where we bind `stderr` to.
`RecLog(DL_Warning, ...` could also be used I guess. Base on my tests destination should not change.
...
A few moments later:
Update:
Thinking this a bit more , seems more consistent to just use `RecLog(DL_Warning, ...` , end goal will be the same. ~~I will update this PR.~~ PR Updated.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#issuecomment-2243122053
Update: use `RecLog(DL_Warning` insted of `Warning` to keep consistency.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1692152528
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
so, to differentiate, which I should have done in the first comment, sorry my bad. This will show something like this:
```
[Jul 16 16:06:30.658] traffic_server WARNING: We have found the following issues when reading the records node:
Error: 'records' root key not present or no fields to read.
Warn: Ignoring field 'invalid_record' at Line 114. Not registered and Unknown tag type '?'
```
So, note the main message:
```
WARNING: We have found the following issues when reading the records node:
```
and then, the subsequently ones which are part of the first warning, but they are error,warning and note in the context of the first warning, and should be read as messages with different severities (Err,WArn,Note, etc) inside the `swoc::Errata`
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1686563739
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,7 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ Warning("%s", swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
Review Comment:
It will go to where we bind `stderr` to. `RecLog(DL_Warning, ...` could also be used I guess. Base on my tests destination should not change.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1690432792
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
Right, I agree Note is not sufficient. Even though Error should indicate that something won't work or is broken, not using the configured records.yaml is bad
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1689812566
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,8 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ RecLog(DL_Warning, "%s",
+ swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
}
} else {
RecLog(DL_Note, "%s does not exist.", g_rec_config_fpath);
Review Comment:
The [above](https://github.com/apache/trafficserver/pull/11560/files#diff-337da9a9ab2e23a59f3648a7015d48326c40944682b481f12d10260df2cda794L238) warning should be already displayed.
I think eventually down the line we may want to make all this more strict.. or maybe not.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1692152528
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
so, to differentiate, which I should have done in the first comment, sorry my bad. This will show something like this:
```
[Jul 16 16:06:30.658] traffic_server WARNING: We have found the following issues when reading the records node:
Error: 'records' root key not present or no fields to read.
Warn: Ignoring field 'invalid_record' at Line 114. Not registered and Unknown tag type '?'
```
So, note the main message:
```
WARNING: We have found the following issues when reading the records node:
```
and then, the subsequently ones which are part of the first warning, but they are error,warning and note in the context of the first warning, and should be read as messages with different severities(Err,WArn,Note, etc)
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1682779412
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,7 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ Warning("%s", swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
Review Comment:
Beyond the wording change, the move from RecLog to Warning will change where the message is printed?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1688422782
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,8 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ RecLog(DL_Warning, "%s",
+ swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
}
} else {
RecLog(DL_Note, "%s does not exist.", g_rec_config_fpath);
Review Comment:
should this be a warning as well? if records.yaml doesn't exist?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1694835959
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
I think we need to find a way to make the `Errata` and the regular log levels to work together, to avoid somehow misleading level of messages like this one can be, this was Alan's idea I guess somehow. I'll try to spend time in the future to work around this.
Thanks for reviewing this @serrislew
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1690441144
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
I was thinking it should be somewhat consistent with your above comment. In that comment, when the file doesn't exist, it uses the default values and uses a warning comment. Here, the records root key doesn't exist so it uses the default values too but it's an error comment.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1688457820
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
how do we differentiate warnings and errors? if TS still runs but without the expected configurations, is that just a warning or error? it was a note before so I'm just curious the jump to error
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1689512930
##########
src/records/RecYAMLDecoder.cc:
##########
@@ -146,15 +146,16 @@ ParseRecordsFromYAML(YAML::Node root, RecYAMLNodeHandler handler, bool lock /*fa
swoc::Errata errata;
if (YAML::NodeType::Map != root.Type()) {
- return swoc::Errata(ERRATA_NOTE, "Node is expected to be a map, got '{}' instead.", root.Type());
+ return swoc::Errata(ERRATA_ERROR, "Node is expected to be a map, got '{}' instead.", root.Type());
}
if (auto ts = root[RECORD_YAML_ROOT_STR]; ts.size()) {
for (auto &&n : ts) {
detail::flatten_node({n.first, n.second, CONFIG_RECORD_PREFIX}, handler, errata);
}
} else {
- return swoc::Errata(ERRATA_NOTE, "'records' root key not present or no fields to read.");
+ return swoc::Errata(ERRATA_ERROR, "'{}' root key not present or no fields to read. Default values will be used",
Review Comment:
The only change will be the text from `Note` to `Error`, the idea is to make it more relevant in the log as not having the `records` node in the config should call someones attention.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 commented on code in PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560#discussion_r1686563739
##########
src/records/RecCore.cc:
##########
@@ -244,7 +244,7 @@ RecCoreInit(Diags *_diags)
RecLog(DL_Note, "records parsing completed.");
if (!err.empty()) {
std::string text;
- RecLog(DL_Note, "%s", swoc::bwprint(text, "{}", err).c_str());
+ Warning("%s", swoc::bwprint(text, "We have found the following issues when reading the records node:\n {}", err).c_str());
Review Comment:
It will go to where we bind `stderr` to.
`RecLog(DL_Warning, ...` could also be used I guess. Base on my tests destination should not change.
...
A few moments later:
Update:
Thinking this a bit more , seems more consistent to just use `RecLog(DL_Warning, ...` , end goal will be the same. I will update this PR.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] records.yaml: Improve the output a little bit to make it clear what's the outcome in some error scenarios. [trafficserver]
Posted by "brbzull0 (via GitHub)" <gi...@apache.org>.
brbzull0 merged PR #11560:
URL: https://github.com/apache/trafficserver/pull/11560
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org