GitHub Issue 1086: Add metrics for some problematic special characters in fields#7808
Conversation
| collectChoiceFieldMetrics(scope, counts); | ||
| collectTextFieldMetrics(scope, dialect, counts, log); | ||
| collectObjectPropertyMetrics(scope, counts); |
There was a problem hiding this comment.
I am a little confused by the subtle differences between the metric values for these different field types. It seems (and maybe I'm just reading the code wrong) that they are counting different things. It seems like text choice fields are counting the total number of fields that have a choice containing special characters (i.e. the choice options not the actual usages of those options) where the text fields are counting the number of distinct values in each row of a field (not the actual number of rows).
I'm just not sure the intended use of this metric to know if this is expected. It just seems like each "metric count" in this set needs to be interpreted a little differently.
There was a problem hiding this comment.
Yes, the metrics for different data types count different things, as documented in https://github.com/LabKey/internal-issues/issues/1086#issuecomment-4861318110. The design decision was based on consideration of implementation complexity and query performance.
| { | ||
| sql.append(first ? " " : ", "); | ||
| first = false; | ||
| sql.append("COUNT(DISTINCT CASE WHEN ("); |
There was a problem hiding this comment.
is the DISTINCT part here intentional? seems like if there were, for example, 10k rows that all had the same value, they would be counted as 1 in this metric (but maybe that is expected)
There was a problem hiding this comment.
The metrics is intended to count "fields" count, not "data rows" count.
| "WHERE dd.storagetablename IS NOT NULL\n" + | ||
| "AND pd.storagecolumnname IS NOT NULL\n" + |
There was a problem hiding this comment.
can these really be null in the DB? I would have expected that they were defined as non-null.
There was a problem hiding this comment.
Yep, those fields are nullable in DB and are often null
|
|
||
| // Sample type and data class provisioned tables have a base "name" column that is not a domain property (so it | ||
| // is absent from exp.propertydescriptor); include it as a Text field. GitHub Issue 1086. | ||
| addBaseNameColumns(scope, byTable); |
There was a problem hiding this comment.
not sure exactly where or why, but when I ran this locally, I hit an exception on my local DB:
org.springframework.jdbc.BadSqlGrammarException: ExecutingSelector; bad SQL grammar []
at org.springframework.jdbc.support.SQLStateSQLExceptionTranslator.doTranslate(SQLStateSQLExceptionTranslator.java:134)
at org.springframework.jdbc.support.AbstractFallbackSQLExceptionTranslator.translate(AbstractFallbackSQLExceptionTranslator.java:102)
at org.springframework.jdbc.support.AbstractFallbackSQLExceptionTranslator.translate(AbstractFallbackSQLExceptionTranslator.java:111)
at org.springframework.jdbc.support.AbstractFallbackSQLExceptionTranslator.translate(AbstractFallbackSQLExceptionTranslator.java:111)
at org.labkey.api.data.ExceptionFramework$1.translate(ExceptionFramework.java:43)
at org.labkey.api.data.SqlExecutingSelector$ExecutingResultSetFactory.handleSqlException(SqlExecutingSelector.java:570)
at org.labkey.api.data.SqlExecutingSelector$ExecutingResultSetFactory.handleResultSet(SqlExecutingSelector.java:469)
at org.labkey.api.data.BaseSelector.getObject(BaseSelector.java:355)
...
Caused by: org.postgresql.util.PSQLException: ERROR: column "name" does not exist
Hint: Perhaps you meant to reference the column "c1603d16551_testassaynab_sample_fields.date".
Position: 849
There was a problem hiding this comment.
Good catch, I'll investigate this.
There was a problem hiding this comment.
As discussed, this is likely due to a corrupted sample type (unknown cause) that's an isolated failure. The metrics job logs this error and proceeds to complete query for other data types, with metrics reported despite isolated errors.
|
|
||
| private void scanTable(DbScope scope, SqlDialect dialect, TableKey table, List<Col> cols, Map<String, Map<String, Long>> counts, Logger log) | ||
| { | ||
| SQLFragment sql = new SQLFragment("SELECT "); |
There was a problem hiding this comment.
I think I might be reading this wrong, but does this select all rows from the provisioned table? Is there a reason that it doesn't do the select distinct approach like the object property metric?
There was a problem hiding this comment.
For provisioned table, each column (field) is queried separately, so there no need to do DISTINCT. For objectproperty, since all fields uses stringvalue column, we need to get the propertyid (field) count.
| } | ||
|
|
||
| @Test | ||
| public void testSpecialCharacterMetrics() throws Exception |
There was a problem hiding this comment.
I guess I wouldn't have expected a junit test for metric code. if the reason that we pushed this to a maintenance task is for performance, would this test case also be rather slow to run (I suppose not in the TeamCity case where we shouldn't have a very large database).
Also, what type of regression would this catch for us? seems like only if the generated SQL was changed, this would fail.
2 participants
Rationale
GitHub Issue 1086.
This PR adds a usage metric so we can understand how frequently some problematic special characters (semicolons, commas, newlines, and double-quotes) actually occur. broken down by special character and by field type.
Related Pull Requests
Changes
SpecialCharacterMetricsMaintenanceTasksystem maintenance task (due to performance concern, PostgreSQL only) that computes, per special character, the number of fields whose values (or defined choices) contain that character, grouped bycategory.
name, which is reported under dataName category.exp.objectpropertybacked domains (assay run, workflow custom fields, etc) don't distinguish between text/multiline, and are reported underobjectStringValue