druid icon indicating copy to clipboard operation
druid copied to clipboard

Fix dataSource dimension decoration in SQLExecutionReporter Metrics

Open ashwintumma23 opened this issue 9 months ago • 2 comments

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 dataSource value 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 the dataSource name in the SQLExecutionReporter itself, so that the decorated values are not carried to the emitters.
    • Side Note: Prometheus Emitter replaces all non-alphanumeric characters with _; so dataSource names in Prometheus will not contain the heading _ and trailing _ automatically!
  • We are also adding a test case to cover this scenario, wherein the expected value of dataSource should be foo. 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: image
  • With the code changes in this PR, the dataSource value is correctly passed as foo

Release note

  • dataSource names for sqlQuery metrics 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
  • SqlExecutionReporter
  • SqlResourceTest

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 avatar Feb 20 '25 09:02 ashwintumma23

@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.

cryptoe avatar Apr 09 '25 10:04 cryptoe

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.

github-actions[bot] avatar Jun 09 '25 00:06 github-actions[bot]

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.

github-actions[bot] avatar Jul 07 '25 00:07 github-actions[bot]