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/10/08 06:47:35 UTC
[GitHub] [skywalking] wankai123 opened a new pull request, #9741: Limit the max length of trace and log tag's `key=value`
wankai123 opened a new pull request, #9741:
URL: https://github.com/apache/skywalking/pull/9741
- [X] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes https://github.com/apache/skywalking/issues/9000.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/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] wu-sheng commented on a diff in pull request #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on code in PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#discussion_r990604771
##########
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SegmentAnalysisListener.java:
##########
@@ -152,6 +152,12 @@ private void appendSearchableTags(SpanObject span) {
span.getTagsList().forEach(tag -> {
if (searchableTagKeys.contains(tag.getKey())) {
final Tag spanTag = new Tag(tag.getKey(), tag.getValue());
+ if (tag.getValue().length() > Tag.TAG_LENGTH || spanTag.toString().length() > Tag.TAG_LENGTH) {
+ if (log.isDebugEnabled()) {
+ log.debug("Segment tag : {} length > : {}, dropped", spanTag, Tag.TAG_LENGTH);
Review Comment:
Warning logs seem a little tricky. Because this kind of log would not disappear unless the user changes business codes or plugin codes. And the warning would not stop and then they would crash the log collecting system.
I think that is not what we expected.
--
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 #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#discussion_r990604111
##########
oap-server/analyzer/agent-analyzer/src/main/java/org/apache/skywalking/oap/server/analyzer/provider/trace/parser/listener/SegmentAnalysisListener.java:
##########
@@ -152,6 +152,12 @@ private void appendSearchableTags(SpanObject span) {
span.getTagsList().forEach(tag -> {
if (searchableTagKeys.contains(tag.getKey())) {
final Tag spanTag = new Tag(tag.getKey(), tag.getValue());
+ if (tag.getValue().length() > Tag.TAG_LENGTH || spanTag.toString().length() > Tag.TAG_LENGTH) {
+ if (log.isDebugEnabled()) {
+ log.debug("Segment tag : {} length > : {}, dropped", spanTag, Tag.TAG_LENGTH);
Review Comment:
I think we should `warn` this, if users can't find search results for those tags, they should be notified that those are dropped instead of thinking there is no such tag at all, otherwise it's hard to diagnose why there is no result in the UI.
--
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] wankai123 commented on a diff in pull request #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
wankai123 commented on code in PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#discussion_r990605238
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/RecordSinkListener.java:
##########
@@ -53,6 +55,7 @@
*/
@RequiredArgsConstructor
public class RecordSinkListener implements LogSinkListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(RecordSinkListener.class);
Review Comment:
Class field `private final Log log = new Log()` conflicts with the `@Slf4j` generated field name `log`, should I change all `log` to another name?
--
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 #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#discussion_r990606603
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/RecordSinkListener.java:
##########
@@ -53,6 +55,7 @@
*/
@RequiredArgsConstructor
public class RecordSinkListener implements LogSinkListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(RecordSinkListener.class);
Review Comment:
> Class field `private final Log log = new Log()` conflicts with the `@Slf4j` generated field name `log`, should I change all `log` to another name?
OK forgot this, let's keep this now
--
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 pull request #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#issuecomment-1272252813
:warning: **6 God Classes** were detected by Lift in this project. [Visit the Lift web console](https://lift.sonatype.com/results/github.com/apache/skywalking/01GEV5JRM08QDQ0RNDJ93YJC2Q?tab=technical-debt&utm_source=github.com&utm_campaign=lift-comment&utm_content=apache\%20skywalking) for more details.
--
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 #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
wu-sheng merged PR #9741:
URL: https://github.com/apache/skywalking/pull/9741
--
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 #9741: Limit the max length of trace and log tag's `key=value`
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on code in PR #9741:
URL: https://github.com/apache/skywalking/pull/9741#discussion_r990604179
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/provider/log/listener/RecordSinkListener.java:
##########
@@ -53,6 +55,7 @@
*/
@RequiredArgsConstructor
public class RecordSinkListener implements LogSinkListener {
+ private static final Logger LOGGER = LoggerFactory.getLogger(RecordSinkListener.class);
Review Comment:
Prefer `@Slf4j` annotation
--
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