Skip to content

Add context dimension to DefaultQueryMetrics#10578

Merged
jihoonson merged 4 commits intoapache:masterfrom
capistrant:default-query-metrics-add-context
Dec 2, 2020
Merged

Add context dimension to DefaultQueryMetrics#10578
jihoonson merged 4 commits intoapache:masterfrom
capistrant:default-query-metrics-add-context

Conversation

@capistrant
Copy link
Copy Markdown
Contributor

@capistrant capistrant commented Nov 13, 2020 •
edited
Loading

Fixes #10577 .

Release Notes

context is now a default dimension emitted for all query metrics. context is a json formatted string containing the query context that is tied to the query that the emitted metric refers to. This will alter some of the metrics that are emitted by Druid (adding a dimension that was not previously there). Clients should plan to handle this new context dimension in their metrics pipeline. Since the dimension is a json formatted string, it is common for clients to parse the dimension and either flatten it or extract the bits they want and discard the full json formatted string blob.

Description

Makes the context dimension a default dimension for DruidQueryMetrics. This will allow clients to gain more informations about the queries producing the metrics.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.
Key changed/added classes in this PR
  • DefaultQueryMetrics

nishantmonu51
nishantmonu51 approved these changes Nov 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM,
NIT: since we moved it to default now,
Can we now remove the context addition in DruidMetrics.makeRequestMetrics ?

jihoonson
jihoonson approved these changes Nov 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR LGTM, but should be called out in the release notes because it can impact on the user metrics store. Also please address #10578 (review).

@capistrant
Copy link
Copy Markdown
Contributor Author

capistrant commented Nov 18, 2020

@nishantmonu51 @jihoonson thank you for the review. I addressed the redundant call to context() in latest commit and added a release notes blurb in the PR description.

@capistrant
Copy link
Copy Markdown
Contributor Author

capistrant commented Nov 18, 2020 •
edited
Loading

Hmm, I appear to be having an inspection problem after my 00efa42 commit. QueryMetrics#context is getting flagged with Method is never used as a member of this interface, but only as a member of the implementation class(es). The project will stay compilable if the method is removed from the interface. ... However, I do not see the difference between this method or any of the other override methods that don't have an inspection error. I'll keep digging, if anyone has some tips, let me know.

Update:

Had to add method to @publicapi in metrics interface now that it is not invoked from outside the implementation class anymore. matches what has been done with the other default dimensions that are not used from outside the class.

jihoonson
jihoonson approved these changes Nov 21, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@capistrant thanks! 👍

@jihoonson jihoonson merged commit 2e02eeb into apache:master Dec 2, 2020
@jihoonson jihoonson added this to the 0.21.0 milestone Jan 4, 2021
@jihoonson jihoonson mentioned this pull request Jan 13, 2021
Closed
@jihoonson
Copy link
Copy Markdown
Contributor

jihoonson commented Jan 14, 2021

@capistrant thanks for the detailed PR description 👍

JulianJaffePinterest pushed a commit to JulianJaffePinterest/druid that referenced this pull request Jan 22, 2021
@capistrant @JulianJaffePinterest
Add context dimension to DefaultQueryMetrics (apache#10578)
196d5bf 
* Add context dimension to DefaultQueryMetrics

* remove redundant addition of context dimension from DruidMetrics now that QueryMetrics adds it by default

* update SearchQueryMetrics to reflect the same pattern as other default dimensions in QueryMetrics

* add PublicApi annotation for context in QueryMetrics Interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jihoonson jihoonson jihoonson approved these changes

@nishantmonu51 nishantmonu51 nishantmonu51 approved these changes

Assignees

No one assigned

Labels

Area - Metrics/Event Emitting Design Review Release Notes

Projects

None yet

Milestone

0.21.0

Development

Successfully merging this pull request may close these issues.

Query Context should be included as a dimension in Query metrics by default

3 participants

@capistrant @jihoonson @nishantmonu51