Fix dataSource dimension decoration in SQLExecutionReporter Metrics
Fixes #17743.
Description
- For the
sqlQuery/*metrics,[and]are added when the data source names are being converted from a List to a String. - This decorated
dataSourcevalue gets emitted by the emitters in decorated format as shown in the screenshots on the linked issue.
Fixed the bug ...
- In this PR, we are explicitly removing the heading
[and trailing]characters from thedataSourcename in theSQLExecutionReporteritself, so that the decorated values are not carried to the emitters.- Side Note: Prometheus Emitter replaces all non-alphanumeric characters with
_; sodataSourcenames in Prometheus will not contain the heading_and trailing_automatically!
- Side Note: Prometheus Emitter replaces all non-alphanumeric characters with
- We are also adding a test case to cover this scenario, wherein the expected value of
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: - With the code changes in this PR, the
dataSourcevalue is correctly passed asfoo
Release 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
SqlExecutionReporterSqlResourceTest
This PR has:
- [X] been self-reviewed.
- [ ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [X] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in licenses.yaml
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
@ashwintumma23 I guess we donot want to change the behavior. I thought of feature flagging this, but it just adds extra config flags which we would have to maintain. Keeping this PR open if we see a lot of traction from the community regrading this PR.
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.