Enable query statement #39990
Enable query statement #39990
Conversation
JJ-WM
requested review from
kushagraThapar,
FabianMeiswinkel,
kirankumarkolli,
xinlian12,
milismsft,
aayush3011,
simorenoh,
jeet1995 and
Pilchie
as code owners
github-actions
bot
added
Community Contribution
|
Thank you for your contribution @JJ-WM! We will review the pull request and get back to you soon. |
| @@ -1295,6 +1396,13 @@ private void verifyOTelTracerAttributes( | |||
|
|
|||
| verifyOTelTracerTransport( | |||
| cosmosDiagnostics, error, mockTracer, enableRequestLevelTracing); | |||
|
|
|||
| if(showQueryStatement) { | |||
| public String getQueryStatement(CosmosDiagnosticsContext ctx) { | ||
| checkNotNull(ctx, "Argument 'ctx' must not be null."); | ||
| return ctx.getQueryStatement(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Do the toJson() call needs to change? Or should there be some JSON ignore annotations on the method above or the query holding field? I.e. if someone calls toJson() should the query be logged out or not? May be not - it can be ok for traces but now the query will show in logs as well. Or that is ok?
There was a problem hiding this comment.
toJson shouldn't print since I didn't add the node there, but it could be an issue if user try to serialize this object directly.
There was a problem hiding this comment.
I think we should add the query text there as well (in the toJson)
There was a problem hiding this comment.
If we add the query text, we could log the query with customer data in the query in the diagnostic does it?
| @@ -1202,6 +1207,10 @@ private StartSpanOptions startSpanOptions(String spanName, CosmosDiagnosticsCont | |||
| !cosmosCtx.getOperationId().equals(ctxAccessor.getSpanName(cosmosCtx))) { | |||
| spanOptions.setAttribute("db.cosmosdb.operation_id", cosmosCtx.getOperationId()); | |||
| } | |||
|
|
|||
| if(showQueryStatement() && null != cosmosCtx.getQueryStatement() ) { | |||
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Channel({} --> {}) closed", channel, this.remoteAddress()); | ||
| } | ||
| logger.info("Channel({} --> {}) closed due to CPU > 0.90D", channel, this.remoteAddress()); |
There was a problem hiding this comment.
Do we need to change this log level? how often the info will be logged out?
Also do check if info level is enabled. If not you will be wasting CPU cycles to do string formatting etc. only for that to be thrown out.
There was a problem hiding this comment.
This one also please revert - too noisy in certain environments - in many scenarios in Spark CPU will continuously be >90% - also the threshold isn't always 90% - it is configurable. If this is important to you, then logging as info once (or if you prefer once every x seconds) would be fine - but not info all the time
| } | ||
| } else { | ||
| final long elapsedTimeInNanos = System.nanoTime() - endpoint.lastRequestNanoTime(); | ||
|
|
||
| if (idleEndpointTimeoutInNanos - elapsedTimeInNanos <= 0) { | ||
| if (logger.isDebugEnabled()) { | ||
| if(logger.isDebugEnabled()) { |
| if (logger.isDebugEnabled()) { | ||
| logger.debug("notifyChannelConnect promise.trySuccess(channel)=false"); | ||
| } | ||
| logger.info("notifyChannelConnect local {}, remote {} promise.trySuccess(channel)=false", channel.localAddress(), channel.remoteAddress()); |
There was a problem hiding this comment.
Why change the log level? Also do check if info level is enabled. If not you will be wasting CPU cycles to do string formatting etc only for that to be thrown out.
There was a problem hiding this comment.
Yeah - this would be too noisy. Please revert the log level change
| if (logger.isDebugEnabled()) { | ||
| logger.debug("notifyChannelConnect future was not successful"); | ||
| } | ||
| logger.error("notifyChannelConnect future was not successful {}", future.cause()); |
sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/models/CosmosClientTelemetryConfig.java
Show resolved
Hide resolved
| * It's the user's responsiblity to sanitize the query if necessary. | ||
| * @return current CosmosClientTelemetryConfig | ||
| */ | ||
| public CosmosClientTelemetryConfig showQueryStatement() { |
There was a problem hiding this comment.
Does the case of turning on/off the showing of queries in traces on the same client (not creating new one) needs to be supported? I.e. dynamic change on/off? Looks like here one can set it on but can't flip it to off.
There was a problem hiding this comment.
I think we should extend this a bit - 1) I think we should allow both adding query when parameterized only (what you have now) - but also allow enabling it for simple query text - so, we would need two flags here - showParameterized, showNonParameterized or so. If those flags are boolean it can be used to enable or disabled. and name of the method should probably be something like setIncludeQueryStatement. I would also argue that this flag should determine whether the query statement is even added to CosmosDiagnosticsContext - and if it is we should also log it (like include in toJson) as well as trace it.
There was a problem hiding this comment.
i was thinking of having too flags also, but that would be too much and decided to move the logic to the user custom tracer implementation. We can discuss more on our call.
…evel base on feed back
…meterized or all queries statement
…meterized or all queries statement
…ne, parameterized or all queries statement" This reverts commit 602a5bf.
…ne, parameterized or all queries statement" This reverts commit 1c1a9f0.
…ne, parameterized or all queries statement" This reverts commit 602a5bf.
…nting none, parameterized or all queries statement"" This reverts commit 4d51243.
…nting none, parameterized or all queries statement"" This reverts commit de71626.
…nting none, parameterized or all queries statement"" This reverts commit 2633f40.
…meterized or all queries statement
… in diagnostic context to json
Description
db.statement are not getting printed in trace according to https://opentelemetry.io/docs/specs/semconv/database/database-spans/ with rational is customer data would be leaked in the query statement. However, users should still have the option to print the query and sanitize it in their own tracing implementation.
This change will add a new options to print query statement in db.statement attribute.
NONE - no query is print
PARAMETERIZED_ONLY - only parameterized query is printed
PARAMETERIZED_AND_NON_PARAMETERIZED - all query is printed.
new CosmosClientTelemetryConfig().showQueryOptions(showQueryOptions);All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines