Add identity to query metrics, logs.#4862
Conversation
Also fix a bug where unauthorized requests would not emit any logs or metrics, and instead would log a "Tried to emit logs and metrics twice" warning. Also rename QueryResource's "getServer" to "cancelQuery", because that's what it does.
|
|
||
| @Override | ||
| public void identity(String identity) | ||
| { |
There was a problem hiding this comment.
I'm still against adding any new dimensions, emitted by default, see #4570 (comment) (and later comments in that thread). Also I committed to contribute something like "full" metrics, with big disclaimer of no backward compatibility: #4570 (comment).
But if you still want to emit it by default, I don't have much motivation to fight against this.
There was a problem hiding this comment.
I thought the potential usefulness of the dimension outweighed the concerns you raised in #4570 (comment), especially since unless you go and specifically set up custom authenticators, the cardinality of the dimension will be 1.
What do you feel about a compromise where this dimension is emitted by default only if the user has authentication configured?
There was a problem hiding this comment.
emitted by default only if the user has authentication configured?
What did you have in mind for the "has authentication configured" check?
There was a problem hiding this comment.
@jon-wei I was thinking checking if the authenticators list is non-default.
There was a problem hiding this comment.
@gianm my main concert is inability to take out or change the dimension in the future, not inefficiency in this case
|
changes LGTM |
|
Thanks @jon-wei for reviewing. I just added a patch to not emit identity by default; everything else is the same. |
|
Thanks @leventov. |
* Add identity to query metrics, logs. Also fix a bug where unauthorized requests would not emit any logs or metrics, and instead would log a "Tried to emit logs and metrics twice" warning. Also rename QueryResource's "getServer" to "cancelQuery", because that's what it does. * Do not emit identity by default.
* Add identity to query metrics, logs. Also fix a bug where unauthorized requests would not emit any logs or metrics, and instead would log a "Tried to emit logs and metrics twice" warning. Also rename QueryResource's "getServer" to "cancelQuery", because that's what it does. * Do not emit identity by default.
* Add identity to query metrics, logs. Also fix a bug where unauthorized requests would not emit any logs or metrics, and instead would log a "Tried to emit logs and metrics twice" warning. Also rename QueryResource's "getServer" to "cancelQuery", because that's what it does. * Do not emit identity by default.
3 participants
Adds "identity" to metrics and logs generated by QueryLifecycle.
Also fix a bug where unauthorized requests would not emit any logs or metrics,
and instead would log a "Tried to emit logs and metrics twice" warning.
Also rename QueryResource's "getServer" to "cancelQuery", because that's what
it does.
@leventov FYI in this patch I added
identityas a dimension that is always part ofquery metrics. I did it for three reasons.