-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collect and log CosmosDB query metrics when extra logging is enabled #4275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4275 +/- ##
==========================================
- Coverage 85.66% 80.88% -4.79%
==========================================
Files 168 168
Lines 7839 7861 +22
Branches 519 523 +4
==========================================
- Hits 6715 6358 -357
- Misses 1124 1503 +379
Continue to review full report at Codecov.
|
} else None | ||
|
||
def collectQueryMetrics(r: FeedResponse[Document]): Unit = { | ||
collectMetrics(queryToken, r.getRequestCharge) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should collectMetrics()
be skipped if queryMetrics.isEmpty
?
nevermind - I see collectMetrics
is just MetricEmitter
val options = newFeedOptions() | ||
val queryMetrics = if (transid.meta.extraLogging) { | ||
options.setPopulateQueryMetrics(true) | ||
Some(scala.collection.mutable.Buffer[QueryMetrics]()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use a var for queryMetrics
, init to List.empty
(or whatever empty collection), and just assign to queryMetrics = r.getQueryMetrics.values()
below, in case extra logging enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be multiple FeedResponse
with each having a set of QueryMetrics
attached and they would need to be accumulated. Hence need to use Buffer
so as to append them and cannot use List.empty
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, replace List.empty
with Buffer[QueryMetrics]()
. I just thought the use of Option
here was confusing - if you don't set options.setPopulateQueryMetrics(true)
, does r.getQueryMetrics.values()
have any values?
If not, you can just always run buffer.appendAll...
).
I guess not creating the buffer at all is slightly better, just harder to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not creating the buffer at all is slightly better, just harder to read.
Yes the objective was to avoid that part for a debug logic. However I see the readability point. Refactored the code to use suggested approach
val combinedMetrics = QueryMetrics.ZERO.add(m: _*) | ||
logging.debug( | ||
this, | ||
s"[QueryMetricsEnabled] Collection [$collName] - Query [${querySpec.getQueryText}].\nQueryMetrics\n[$combinedMetrics]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what shows up in $combinedMetrics
- do you have some sample output? I don't see it in the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample output is there in PR description. The testcase currently only looks for a marker QueryMetricsEnabled
to assert that log statement is executed
a651847
to
49024e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM Thanks! |
…pache#4275) * Collect and log CosmosDB query metrics when extra logging is enabled Fixes apache#4268
Collect and logs CosmosDB query metrics when extra logging is enabled
Fixes #4268
Description
if a request is performed with
X-OW-EXTRA-LOGGING
header set toon
like belowThen in the logs following entries would logged which provide detailed insight into how query is executed by CosmosDB
Related issue and scope
My changes affect the following components
Types of changes
Checklist: