You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wu-sheng (via GitHub)" <gi...@apache.org> on 2023/03/08 07:47:50 UTC
[GitHub] [skywalking-query-protocol] wu-sheng opened a new pull request, #110: Add isEmptyValue flag to indicate clear meaning for value
wu-sheng opened a new pull request, #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110
This will be a part of 9.5.0 release only. Submit for the review 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-query-protocol] wu-sheng merged pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110
--
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-query-protocol] toffentoffen commented on a diff in pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110#discussion_r1141989257
##########
metric.graphqls:
##########
@@ -50,6 +50,8 @@ type KVInt {
# 1. If ask for cpm metric, the unit and result should be count.
# 2. If ask for response time (p99 or avg), the unit should be millisecond.
value: Long!
+ # Since 9.5.0, isEmptyValue indicates whether value == 0 represents actually zero(false, default) or no data(true).
+ isEmptyValue: Boolean!
Review Comment:
Sorry for joining late.
Question: Does it make sense to **only** populate(set to true) when there is no data?
I guess that to make clients life easier it is better to always have that value present. I was just thinking about the payload increase in terms of size.
--
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-query-protocol] toffentoffen commented on a diff in pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110#discussion_r1142410342
##########
metric.graphqls:
##########
@@ -50,6 +50,8 @@ type KVInt {
# 1. If ask for cpm metric, the unit and result should be count.
# 2. If ask for response time (p99 or avg), the unit should be millisecond.
value: Long!
+ # Since 9.5.0, isEmptyValue indicates whether value == 0 represents actually zero(false, default) or no data(true).
+ isEmptyValue: Boolean!
Review Comment:
If the return `value: 0` is not a real `0`, then set the `emptyValue=true`. Otherwise, when the value is a real `0` or `>0` don't even return the `emptyValue` property in the marshalled response.
But as I said, this could add unnecessary complexity at both server side and client side code.
--
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-query-protocol] toffentoffen commented on a diff in pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "toffentoffen (via GitHub)" <gi...@apache.org>.
toffentoffen commented on code in PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110#discussion_r1142410342
##########
metric.graphqls:
##########
@@ -50,6 +50,8 @@ type KVInt {
# 1. If ask for cpm metric, the unit and result should be count.
# 2. If ask for response time (p99 or avg), the unit should be millisecond.
value: Long!
+ # Since 9.5.0, isEmptyValue indicates whether value == 0 represents actually zero(false, default) or no data(true).
+ isEmptyValue: Boolean!
Review Comment:
If the return `value: 0` is not a real `0`, then set the `emptyValue=true`. Otherwise, when the value is a real `0` or `>0` don't even return the `emptyValue` property in the marshalled response.
But as I said, this could add unnecessary complexity at both server side and client side code to just safe some bytes in the response 😄 .
--
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-query-protocol] wu-sheng commented on a diff in pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110#discussion_r1142809824
##########
metric.graphqls:
##########
@@ -50,6 +50,8 @@ type KVInt {
# 1. If ask for cpm metric, the unit and result should be count.
# 2. If ask for response time (p99 or avg), the unit should be millisecond.
value: Long!
+ # Since 9.5.0, isEmptyValue indicates whether value == 0 represents actually zero(false, default) or no data(true).
+ isEmptyValue: Boolean!
Review Comment:
> If the return value: 0 is not a real 0, then set the emptyValue=true. Otherwise, when the value is a real 0 or >0 don't even return the emptyValue property in the marshalled response.
That may not be necessary, as the client does/will always require this field, as they don't know when it comes with 0. And this costs very little on the server side actually. :) We don't need to worry about that.
--
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-query-protocol] wu-sheng commented on a diff in pull request #110: Add isEmptyValue flag to indicate clear meaning for value
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #110:
URL: https://github.com/apache/skywalking-query-protocol/pull/110#discussion_r1142037090
##########
metric.graphqls:
##########
@@ -50,6 +50,8 @@ type KVInt {
# 1. If ask for cpm metric, the unit and result should be count.
# 2. If ask for response time (p99 or avg), the unit should be millisecond.
value: Long!
+ # Since 9.5.0, isEmptyValue indicates whether value == 0 represents actually zero(false, default) or no data(true).
+ isEmptyValue: Boolean!
Review Comment:
You could only use it when the value is `0`. But from a server-side perspective, it must be returned from a GraphQL perspective. I am not sure what do you expect about `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