You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "leon756 (via GitHub)" <gi...@apache.org> on 2023/06/25 06:14:35 UTC
[GitHub] [skywalking] leon756 opened a new pull request, #10990: fix: Limit the length of Event#parameters(#10263)
leon756 opened a new pull request, #10990:
URL: https://github.com/apache/skywalking/pull/10990
### Fix <bug description or the bug issue number or bug issue link>
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it:
Keep the JSON format when substring
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #10263.
- [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 closed pull request #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #10990: fix: Limit the length of Event#parameters(#10263)
URL: https://github.com/apache/skywalking/pull/10990
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1614272666
> > @kezhenxu94 I also want to push a little back about this fix. What do you think we make this longer, than in the storage level, it would be a `text` rather than a varchar. Considering `storageOnly = true`, it should be harmless. Then we don't need this fix, but a type change only.
>
> Changing the type of Event#parameters is good for ElasticSearch storage, but for JDBC storage it seems to have no effect.
I remember if it is oversize, it should be `text` type in JDBC. Isn't it?
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1615922512
@leon756 Are you following this?
--
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] leon756 commented on a diff in pull request #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "leon756 (via GitHub)" <gi...@apache.org>.
leon756 commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241064143
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Event.java:
##########
@@ -153,8 +154,15 @@ public boolean combine(final Metrics metrics) {
* @since 9.0.0 Limit the length of {@link #parameters}
*/
public void setParameters(String parameters) {
- this.parameters = parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH ?
- parameters : parameters.substring(0, PARAMETER_MAX_LENGTH);
+ if (parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH) {
+ this.parameters = parameters;
+ } else {
+ try {
+ this.parameters = StringUtil.trimJson(parameters, PARAMETER_MAX_LENGTH);
+ } catch (Exception e) {
Review Comment:
If not, `JsonParser.parseString(jsonString)` may throw `JsonSyntaxException`
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241064519
##########
oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/StringUtilTest.java:
##########
@@ -76,4 +76,19 @@ public void testTrim() {
assertEquals(StringUtil.trim("abcdef", 'a'), "bcdef");
assertEquals(StringUtil.trim("abcdef", 'f'), "abcde");
}
+
+ @Test
+ public void testTrimJson() {
Review Comment:
This UT is testing flat JSON, how about JSON with a hierarchy structure?
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1614276678
This is for JDBC(H2), check MySQLTableInstaller
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241049386
##########
oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/StringUtilTest.java:
##########
@@ -76,4 +76,12 @@ public void testTrim() {
assertEquals(StringUtil.trim("abcdef", 'a'), "bcdef");
assertEquals(StringUtil.trim("abcdef", 'f'), "abcde");
}
+
+ @Test
+ public void testTrimJson() {
+ String jsonString = "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true,\"k4\":[{\"a\":\"b\",\"c\":\"d\"},{\"e\":\"f\",\"g\":\"h\"}]}";
+ assertEquals(StringUtil.trimJson(jsonString, 10), "{\"k1\":\"v1\",\"k2\":\"v2\"}");
+ assertEquals(StringUtil.trimJson(jsonString, 30), "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true}");
+ }
Review Comment:
I think we should verify whether the size of the trimmed JSON str oversized.
##########
oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/StringUtilTest.java:
##########
@@ -76,4 +76,12 @@ public void testTrim() {
assertEquals(StringUtil.trim("abcdef", 'a'), "bcdef");
assertEquals(StringUtil.trim("abcdef", 'f'), "abcde");
}
+
+ @Test
+ public void testTrimJson() {
+ String jsonString = "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true,\"k4\":[{\"a\":\"b\",\"c\":\"d\"},{\"e\":\"f\",\"g\":\"h\"}]}";
+ assertEquals(StringUtil.trimJson(jsonString, 10), "{\"k1\":\"v1\",\"k2\":\"v2\"}");
+ assertEquals(StringUtil.trimJson(jsonString, 30), "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true}");
+ }
Review Comment:
I think we should verify whether the size of the trimmed JSON str is oversized.
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1615943442
Fixed by https://github.com/apache/skywalking/pull/11032
--
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] leon756 commented on a diff in pull request #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "leon756 (via GitHub)" <gi...@apache.org>.
leon756 commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241063557
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Event.java:
##########
@@ -153,8 +154,15 @@ public boolean combine(final Metrics metrics) {
* @since 9.0.0 Limit the length of {@link #parameters}
*/
public void setParameters(String parameters) {
- this.parameters = parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH ?
- parameters : parameters.substring(0, PARAMETER_MAX_LENGTH);
+ if (parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH) {
+ this.parameters = parameters;
+ } else {
+ try {
+ this.parameters = StringUtil.trimJson(parameters, PARAMETER_MAX_LENGTH);
+ } catch (Exception e) {
Review Comment:
I am not quite sure that the original parameters must be in json format
--
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] leon756 commented on a diff in pull request #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "leon756 (via GitHub)" <gi...@apache.org>.
leon756 commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241062843
##########
oap-server/server-library/library-util/src/test/java/org/apache/skywalking/oap/server/library/util/StringUtilTest.java:
##########
@@ -76,4 +76,12 @@ public void testTrim() {
assertEquals(StringUtil.trim("abcdef", 'a'), "bcdef");
assertEquals(StringUtil.trim("abcdef", 'f'), "abcde");
}
+
+ @Test
+ public void testTrimJson() {
+ String jsonString = "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true,\"k4\":[{\"a\":\"b\",\"c\":\"d\"},{\"e\":\"f\",\"g\":\"h\"}]}";
+ assertEquals(StringUtil.trimJson(jsonString, 10), "{\"k1\":\"v1\",\"k2\":\"v2\"}");
+ assertEquals(StringUtil.trimJson(jsonString, 30), "{\"k1\":\"v1\",\"k2\":\"v2\",\"k3\":true}");
+ }
Review Comment:
Sorry, I was careless.
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241063210
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Event.java:
##########
@@ -153,8 +154,15 @@ public boolean combine(final Metrics metrics) {
* @since 9.0.0 Limit the length of {@link #parameters}
*/
public void setParameters(String parameters) {
- this.parameters = parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH ?
- parameters : parameters.substring(0, PARAMETER_MAX_LENGTH);
+ if (parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH) {
+ this.parameters = parameters;
+ } else {
+ try {
+ this.parameters = StringUtil.trimJson(parameters, PARAMETER_MAX_LENGTH);
+ } catch (Exception e) {
Review Comment:
I can't see the point of this catch. What do you want to do about this case? When is the exception thrown?
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#discussion_r1241064119
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/metrics/Event.java:
##########
@@ -153,8 +154,15 @@ public boolean combine(final Metrics metrics) {
* @since 9.0.0 Limit the length of {@link #parameters}
*/
public void setParameters(String parameters) {
- this.parameters = parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH ?
- parameters : parameters.substring(0, PARAMETER_MAX_LENGTH);
+ if (parameters == null || parameters.length() <= PARAMETER_MAX_LENGTH) {
+ this.parameters = parameters;
+ } else {
+ try {
+ this.parameters = StringUtil.trimJson(parameters, PARAMETER_MAX_LENGTH);
+ } catch (Exception e) {
Review Comment:
@kezhenxu94 Is the JSON format required by the protocol?
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1605903671
@kezhenxu94 I also want to push a little back about this fix. What do you think we make this longer, than in the storage level, it would be a `text` rather than a varchar. Considering `storageOnly = true`, it should be harmless. Then we don't need this fix, but a type change only.
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1614274922
> > > @kezhenxu94 I also want to push a little back about this fix. What do you think we make this longer, than in the storage level, it would be a `text` rather than a varchar. Considering `storageOnly = true`, it should be harmless. Then we don't need this fix, but a type change only.
> >
> >
> > Changing the type of Event#parameters is good for ElasticSearch storage, but for JDBC storage it seems to have no effect.
>
> I remember if it is oversize, it should be `text` type in JDBC. Isn't it?
Only field of type `byte[]` is `mediumtext` in JDBC storage.
https://github.com/apache/skywalking/blob/ffc8453332bc991e97ad4c745e56437b5464e875/oap-server/server-storage-plugin/storage-jdbc-hikaricp-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/jdbc/common/JDBCTableInstaller.java#L107-L129
--
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 #10990: fix: Limit the length of Event#parameters(#10263)
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10990:
URL: https://github.com/apache/skywalking/pull/10990#issuecomment-1614271016
> @kezhenxu94 I also want to push a little back about this fix. What do you think we make this longer, than in the storage level, it would be a `text` rather than a varchar. Considering `storageOnly = true`, it should be harmless. Then we don't need this fix, but a type change only.
Changing the type of Event#parameters is good for ElasticSearch storage, but for JDBC storage it seems to have no effect.
--
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