Removing full exception trace from 404 error #24195

simplynaveen20 · 2021-09-17T15:53:23Z

This PR contains below changes

Removed the full error stack from customer tracer on 404 error as it consider a regular business scenario , this will reduce noise and cost for customer in App Insight/Reporters
Also removing explicit diagnostics information from span events for all exceptions which was added in Integrating cosmos diagnostics with open telemetry tracer #22202 , complete diagnostics are already captured in exception event

Before the change

After the change

moderakh

shouldn't we do the same thing for 409s (ResourceAlreadyExists) as well?

other than that LGTM. thanks @simplynaveen20

moderakh · 2021-09-17T16:36:09Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/TracerProvider.java

@@ -289,10 +281,18 @@ public void addEvent(String name, Map<String, Object> attributes, OffsetDateTime

    private void end(int statusCode, Throwable throwable, Context context) {
        if (throwable != null) {
-            tracer.setAttribute(TracerProvider.ERROR_MSG, throwable.getMessage(), context);
-            tracer.setAttribute(TracerProvider.ERROR_TYPE, throwable.getClass().getName(), context);
+            if (statusCode == HttpConstants.StatusCodes.NOTFOUND) {


shouldn't we do the same thing for 409s (ResourceAlreadyExists) as well?

We talked through all these and decided 404 is more regular business scenario then others. We will wait and watch for other error on customer demand basis

FabianMeiswinkel · 2021-09-17T16:37:17Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/TracerProvider.java

@@ -289,10 +281,18 @@ public void addEvent(String name, Map<String, Object> attributes, OffsetDateTime

    private void end(int statusCode, Throwable throwable, Context context) {
        if (throwable != null) {
-            tracer.setAttribute(TracerProvider.ERROR_MSG, throwable.getMessage(), context);
-            tracer.setAttribute(TracerProvider.ERROR_TYPE, throwable.getClass().getName(), context);
+            if (statusCode == HttpConstants.StatusCodes.NOTFOUND) {


Please only remove the callstack etc. for 404 with Substatusode 0 - for any SubStatusCode != 0 (like ReadSessionNotAvailable 1002 etc.) we should leave the callstack

Yea sure will add substatus check as well.

Done, added 404/0 check

FabianMeiswinkel

LGTM except for the 404 with SubStatusCode != 0

lmolkova · 2021-09-17T17:46:55Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/TracerProvider.java

+                tracer.setAttribute(TracerProvider.ERROR_TYPE, throwable.getClass().getName(), context);
+                tracer.end(statusCode, null, context);
+            } else {
+                tracer.setAttribute(TracerProvider.ERROR_MSG, throwable.getMessage(), context);


BTW, why are we adding those?

tracer.end will do span.recordException(t)

https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/implementation/HttpTraceUtil.java#L53

which internally record common exception attributes
https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java#L406-L409

So we can remove all ERROR_TYPE/ERROR_MSG attributes logic?

If we remove ERROR_TYPE/ERROR_MSG then we wont be seeing below attributes which was the initial requirement from @trask

Can you please add screenshot for non-404 case? There will be standard otel attributes recorded for exception (exception.type and exceptions.message)?

@trask I believe you wanted to have otel semantic exception attributes attached, they have changed and now we are populating new ones + old ones. Are you ok with cosmos removing error.msg and error.type? Do you believe we use them anywhere?

yes, I think we should remove those, I had similar thought a few weeks ago but got distracted from opening PR 😓: trask@af44c69

@trask Removed error.msg and error.type.
@lmolkova please see the screen shot when it is not 404/0 (item or collection not found). We cannot differentiate between item and collection not found without parsing cosmos exception diagnostics and it will be error prone in future.
However if the customer logging the exception they can still see the full diagnostics and differentiate

lmolkova · 2021-09-17T17:50:27Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/TracerProvider.java

-            tracer.setAttribute(TracerProvider.ERROR_MSG, throwable.getMessage(), context);
-            tracer.setAttribute(TracerProvider.ERROR_TYPE, throwable.getClass().getName(), context);
+            if (statusCode == HttpConstants.StatusCodes.NOTFOUND) {
+                tracer.setAttribute(TracerProvider.ERROR_MSG, "Not found exception", context);


I believe 404 statusCode is enough and the core tracer will populate all the info needed.

https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/core/azure-core-tracing-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/implementation/HttpTraceUtil.java#L179

Can we avoid populating custom error codes?

If we remove ERROR_TYPE/ERROR_MSG then we wont be seeing below attributes which was the initial requirement from @trask

since this is no longer an exception, it shouldn't have exception attributes - otel.status_description gives user the info they need

If we remove ERROR_TYPE/ERROR_MSG then we wont be seeing below attributes which was the initial requirement from @trask

this was probably before I understood the otel semantic convention for exceptions 😅

@lmolkova is correct that recordException() is the right way to attach the exception data

Removed error.msg and error.type

Added 404/0 filter

Removing full exception trace from 404 error

808c628

simplynaveen20 requested review from aayush3011, FabianMeiswinkel, kirankumarkolli, kushagraThapar, mbhaskar, milismsft, moderakh and xinlian12 as code owners September 17, 2021 15:53

ghost added the Cosmos label Sep 17, 2021

simplynaveen20 requested review from lmolkova and trask September 17, 2021 15:53

moderakh approved these changes Sep 17, 2021

View reviewed changes

FabianMeiswinkel reviewed Sep 17, 2021

View reviewed changes

FabianMeiswinkel previously requested changes Sep 17, 2021

View reviewed changes

lmolkova reviewed Sep 17, 2021

View reviewed changes

Adding 404/0 filter

f9c50c9

simplynaveen20 requested a review from FabianMeiswinkel September 20, 2021 20:25

removing redudant error.msg and error.type

6c0af33

lmolkova approved these changes Sep 22, 2021

View reviewed changes

simplynaveen20 merged commit 240d0f3 into Azure:main Sep 22, 2021

lmolkova mentioned this pull request Nov 12, 2021

Remove ErrorOptions #25338

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing full exception trace from 404 error #24195

Removing full exception trace from 404 error #24195

simplynaveen20 commented Sep 17, 2021

moderakh left a comment

moderakh Sep 17, 2021

FabianMeiswinkel Sep 17, 2021

simplynaveen20 Sep 17, 2021

FabianMeiswinkel Sep 17, 2021

simplynaveen20 Sep 17, 2021

simplynaveen20 Sep 20, 2021

FabianMeiswinkel left a comment

lmolkova Sep 17, 2021 •

edited

Loading

simplynaveen20 Sep 20, 2021

lmolkova Sep 20, 2021 •

edited

Loading

trask Sep 20, 2021

simplynaveen20 Sep 21, 2021

lmolkova Sep 17, 2021

simplynaveen20 Sep 20, 2021

lmolkova Sep 20, 2021

trask Sep 20, 2021

simplynaveen20 Sep 21, 2021

Removing full exception trace from 404 error #24195

Removing full exception trace from 404 error #24195

Conversation

simplynaveen20 commented Sep 17, 2021

moderakh 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

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

lmolkova Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Sep 20, 2021 • edited Loading

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

lmolkova Sep 17, 2021 •

edited

Loading

lmolkova Sep 20, 2021 •

edited

Loading