Fix dataSource dimension decoration in SQLExecutionReporter Metrics #17744
Conversation
| .stream() | ||
| .map(action -> action.getResource().getName()) | ||
| .collect(Collectors.toList()) | ||
| .toString(); |
There was a problem hiding this comment.
I was looking into this a bit; it seems odd to remove the [ with string operations;
actually there is a setDimension method which may accept a List
I wonder if it would be a better fix be to remove this toString() here and fix the PrometheusEmitter to alter the serialization of List -s to do adjustments there?
There was a problem hiding this comment.
Hi @kgyrtkirk ,
That's right, setDimension(String dim, Object value) can accept a List as well.
About fixing it in the PrometheusEmitter: Initially, even I started with the same thought process, but later realized that the issue impacts all the emitters. A few examples (Prometheus, Kafka, Logging) cited in #17743 . Hence, decided to perform the string clean up operation here.
There was a problem hiding this comment.
wow; didn't know about that - then you might want to fix the handling of when the passed value is List in the builder to deliver it for all cases - and not just for this one case?
note: I wonder how the case when the list has a "," in it should be handled...
There was a problem hiding this comment.
By Builder, do you mean, stmt.authResult.sqlResourceActions above? This is a Set of ResourceActions; so we will have to iterate on that and form a list.
This PR will fix the metrics coming from SQL query execution only. For other metrics, there's the separate issue that we are working on.
note: I wonder how the case when the list has a "," in it should be handled...
Currently, we see that a list that has , in it, gets translated to "[a,b]" as a string. With the change in this PR, it will be "a,b". Furthermore, Prometheus Emitter replaces all non alphanumeric and non underscore characters with _; so it would look like "_a_b_"
There was a problem hiding this comment.
Hi @kgyrtkirk, any thoughts on the above comment? Thanks!
There was a problem hiding this comment.
41712b7
Looks like this has been the behavior for a long time.
Changing it here means its a backward incompatible change since dashboards start breaking.
There was a problem hiding this comment.
+1, I think the existing behaviour is fine the way it is.
I don't think Druid docs mention anywhere that the values of a dimension must adhere to the same format across different metrics.
Fixes #17743.
Description
sqlQuery/*metrics,[and]are added when the data source names are being converted from a List to a String.dataSourcevalue gets emitted by the emitters in decorated format as shown in the screenshots on the linked issue.Fixed the bug ...
[and trailing]characters from thedataSourcename in theSQLExecutionReporteritself, so that the decorated values are not carried to the emitters._; sodataSourcenames in Prometheus will not contain the heading_and trailing_automatically!dataSourceshould befoo. Before making the code change, I ran the unit test on the current codebase, and I can clearly see the actual value was[foo]. Attaching reference screenshot here:dataSourcevalue is correctly passed asfooRelease note
dataSourcenames forsqlQuerymetrics will not include heading[and trailing]or any other character decorations; making them consistent across other Druid metrics.Key changed/added classes in this PR
SqlExecutionReporterSqlResourceTestThis PR has: