Open Telemetry : Adds query text in attribute #4664

sourabh1007 · 2024-09-04T17:16:40Z

Description

Added db.query.text attribute to record queries in Traces.
Introduced new ShowQueryMode, which will have valid values as
a) NONE :Do not show query.
b) PARAMETERIZED_ONLY : Print parameterized query only.
b) ALL
It can be set as part of CosmosClientTelemetryOptions and RequestOptions (i.e. QueryRequestOptions and ChangeFeedRequestOptions)

Type of change

[] New feature (non-breaking change which adds functionality)

Microsoft.Azure.Cosmos/src/ChangeFeed/ChangeFeedQuerySpec.cs

jcocchi

Do we want to have an opt in to also capture parameter values attributes? This is described in the otel conventions, but we can always go back and add it if there's demand later.

db.query.parameter.<key>

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryRecorderFactory.cs

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/ShowQueryMode.cs

Microsoft.Azure.Cosmos/src/ChangeFeed/ChangeFeedIteratorCore.cs

Microsoft.Azure.Cosmos/src/CosmosClientTelemetryOptions.cs

kirankumarkolli · 2024-09-24T20:56:40Z

Reqeustlevel override feels over engineering, Thoughts?

sourabh1007 · 2024-09-25T09:04:25Z

verride feels over engineering, Thoughts?

It is taken from Java SDK and I believe, It will provide customers with the option to enable or disable the inclusion of query text in OpenTelemetry (OTel) traces for specific types of queries.

philipthomas-MSFT

Nothing major from me. LGTM. just one nit. Everything else looks good.

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMode.cs

Microsoft.Azure.Cosmos/src/RequestOptions/ChangeFeedRequestOptions.cs

kirankumarkolli · 2024-09-26T14:27:57Z

Microsoft.Azure.Cosmos/src/CosmosClientTelemetryOptions.cs

+        /// Users have the option to enable printing parameterized or all queries, 
+        /// but has to beware that customer data may be shown when the later option is chosen. It's the user's responsibility to sanitize the queries if necessary.
+        /// </summary>
+        public QueryTextMode QueryTextMode { get; set; } = QueryTextMode.None;


Inconsistency, in-other request options nullable used

what is the inconsistency, you are talking about here. All the telemetry options have some default value.

This default value makes sense to me

Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator.cs

Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator{T}.cs

kirankumarkolli · 2024-09-26T14:30:20Z

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs

@@ -26,6 +26,7 @@ internal struct OpenTelemetryCoreRecorder : IDisposable
        private readonly OperationType operationType = OperationType.Invalid;
        private readonly string connectionModeCache = null;

+        private readonly QueryTextMode queryTextMode = QueryTextMode.None;


Mark it nullable for consistency and only apply default here?

kirankumarkolli · 2024-09-26T14:31:09Z

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs

@@ -245,6 +250,16 @@ OperationType operationType
                    this.scope.AddAttribute(OpenTelemetryAttributeKeys.ActivityId, this.response.ActivityId);
                    this.scope.AddAttribute(OpenTelemetryAttributeKeys.CorrelatedActivityId, this.response.CorrelatedActivityId);

+                    if (this.response.QuerySpec is not null)


Suggested change

if (this.response.QuerySpec is not null)

if (this.response.QuerySpec.HasValue)

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs

kirankumarkolli · 2024-09-26T14:50:28Z

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs

+                            (this.queryTextMode != QueryTextMode.None && this.response.QuerySpec.ShouldSerializeParameters() && 
+                             this.queryTextMode == QueryTextMode.ParameterizedOnly))
+                        {
+                            this.scope.AddAttribute(OpenTelemetryAttributeKeys.QueryText, this.response.QuerySpec?.QueryText);


Expected semantics: Is below expectation correct? If so code needs to aligns with it.

All: Full query text including parameterized with parameter values

Paramaterized: Query with-out parameters?

All => The entire query will be included, regardless of whether it is parameterized or not.
Parameterized Only => Only parameterized queries will be included. If the query contains any hardcoded information, it will also be included as is. It is the customer's responsibility to ensure that the query does not contain any sensitive information.
None => The query text will not be included.

regardless of whether it is parameterized or not

Sounds like if it is parameterized, we won't "fill" the parameter with real values. I left a comment on this already, but putting it again here since it's relevant. Do we want to have an opt in to also capture parameter values attributes? This is described in the otel conventions, but we can always omit for now and add it if there's demand later.

db.query.parameter.

Adding parameter is kind of unlikely and in Java SDK never demanded for it, so I think we should be good as of now, if we see customers requesting for it, we can include it.

…{T}.cs Co-authored-by: Kiran Kumar Kolli <[email protected]>

Co-authored-by: Kiran Kumar Kolli <[email protected]>

…ryCoreRecorder.cs Co-authored-by: Kiran Kumar Kolli <[email protected]>

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMode.cs

jcocchi · 2024-09-26T23:11:16Z

Microsoft.Azure.Cosmos/src/CosmosClientTelemetryOptions.cs

+        /// Users have the option to enable printing parameterized or all queries, 
+        /// but has to beware that customer data may be shown when the later option is chosen. It's the user's responsibility to sanitize the queries if necessary.
+        /// </summary>
+        public QueryTextMode QueryTextMode { get; set; } = QueryTextMode.None;


This default value makes sense to me

…de.cs Co-authored-by: Justine Cocchi <[email protected]>

jcocchi

LGTM

sourabh1007 requested review from khdang, sboshra, adityasa, neildsh, kirankumarkolli, FabianMeiswinkel and kirillg as code owners September 4, 2024 17:16

sourabh1007 marked this pull request as draft September 4, 2024 17:16

sourabh1007 force-pushed the users/sourabhjain/addqueryTextinOtel branch 3 times, most recently from 2f7c8e7 to e663af5 Compare September 11, 2024 01:24

sourabh1007 marked this pull request as ready for review September 12, 2024 00:45

sourabh1007 requested a review from a team as a code owner September 12, 2024 00:45

NaluTripician reviewed Sep 12, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/ChangeFeed/ChangeFeedQuerySpec.cs Outdated Show resolved Hide resolved

sourabh1007 force-pushed the users/sourabhjain/addqueryTextinOtel branch from 816b162 to 8e29f00 Compare September 13, 2024 12:32

jcocchi reviewed Sep 17, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs Show resolved Hide resolved

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryRecorderFactory.cs Outdated Show resolved Hide resolved

sourabh1007 force-pushed the users/sourabhjain/addqueryTextinOtel branch from 9b3858e to 35201c3 Compare September 18, 2024 02:11

sourabh1007 requested a review from Pilchie as a code owner September 18, 2024 02:11

joegoldman2 reviewed Sep 20, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/ShowQueryMode.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 24, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/ChangeFeed/ChangeFeedIteratorCore.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 24, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/CosmosClientTelemetryOptions.cs Outdated Show resolved Hide resolved

sourabh1007 force-pushed the users/sourabhjain/addqueryTextinOtel branch from 6343391 to 23b13bf Compare September 25, 2024 10:23

philipthomas-MSFT previously approved these changes Sep 25, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMode.cs Outdated Show resolved Hide resolved

sourabh1007 dismissed philipthomas-MSFT’s stale review via 5fd9ce8 September 26, 2024 13:54

sourabh1007 added 4 commits September 26, 2024 19:24

first draft

b596c48

fix query iterator

27d3f8c

fixing test

f4705b1

enable show query mode

e49f87a

sourabh1007 added 7 commits September 26, 2024 19:24

fix tests

7c64cfb

changefeed fix

38a50a8

fix changefeed

a58fe18

fix imports

48135cf

added querymode

78b8144

feedback changes

69aedb2

change text casing in querytext node

b760982

sourabh1007 force-pushed the users/sourabhjain/addqueryTextinOtel branch from 5fd9ce8 to b760982 Compare September 26, 2024 13:54

kirankumarkolli reviewed Sep 26, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/RequestOptions/ChangeFeedRequestOptions.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 26, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 26, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator{T}.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 26, 2024

View reviewed changes

Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemetryCoreRecorder.cs Outdated Show resolved Hide resolved

kirankumarkolli reviewed Sep 26, 2024

View reviewed changes

sourabh1007 and others added 4 commits September 26, 2024 21:10

Update Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator…

fdd6715

…{T}.cs Co-authored-by: Kiran Kumar Kolli <[email protected]>

Update Microsoft.Azure.Cosmos/src/Resource/FeedIterators/FeedIterator.cs

423e391

Co-authored-by: Kiran Kumar Kolli <[email protected]>

Update Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/OpenTelemet…

70047ab

…ryCoreRecorder.cs Co-authored-by: Kiran Kumar Kolli <[email protected]>

refactor code

a991c90

jcocchi reviewed Sep 26, 2024

View reviewed changes

sourabh1007 and others added 4 commits September 27, 2024 06:14

Update Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMo…

b064d37

…de.cs Co-authored-by: Justine Cocchi <[email protected]>

Update Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMo…

a5b7153

…de.cs Co-authored-by: Justine Cocchi <[email protected]>

Update Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMo…

8c048a3

…de.cs Co-authored-by: Justine Cocchi <[email protected]>

Update Microsoft.Azure.Cosmos/src/Telemetry/OpenTelemetry/QueryTextMo…

f587ab3

…de.cs Co-authored-by: Justine Cocchi <[email protected]>

jcocchi previously approved these changes Sep 27, 2024

View reviewed changes

sourabh1007 added 2 commits September 28, 2024 15:42

Merge branch 'master' into users/sourabhjain/addqueryTextinOtel

6feb8b8

contract fix

54520fa

sourabh1007 dismissed jcocchi’s stale review via 54520fa September 29, 2024 01:28

Merge branch 'master' into users/sourabhjain/addqueryTextinOtel

70d93e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open Telemetry : Adds query text in attribute #4664

Open Telemetry : Adds query text in attribute #4664

sourabh1007 commented Sep 4, 2024 •

edited

Loading

jcocchi left a comment

kirankumarkolli commented Sep 24, 2024

sourabh1007 commented Sep 25, 2024

philipthomas-MSFT left a comment

kirankumarkolli Sep 26, 2024

sourabh1007 Sep 26, 2024

jcocchi Sep 26, 2024

kirankumarkolli Sep 26, 2024

kirankumarkolli Sep 26, 2024

kirankumarkolli Sep 26, 2024

sourabh1007 Sep 26, 2024

jcocchi Sep 26, 2024

sourabh1007 Sep 27, 2024

jcocchi Sep 26, 2024

jcocchi left a comment

	if (this.response.QuerySpec is not null)
	if (this.response.QuerySpec.HasValue)

Open Telemetry : Adds query text in attribute #4664

Are you sure you want to change the base?

Open Telemetry : Adds query text in attribute #4664

Conversation

sourabh1007 commented Sep 4, 2024 • edited Loading

Description

Type of change

jcocchi left a comment

Choose a reason for hiding this comment

kirankumarkolli commented Sep 24, 2024

sourabh1007 commented Sep 25, 2024

philipthomas-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcocchi left a comment

Choose a reason for hiding this comment

sourabh1007 commented Sep 4, 2024 •

edited

Loading