XML/SB/Agent configuration for exceptions truncation by vlev · Pull Request #806 · microsoft/ApplicationInsights-Java

vlev · 2019-01-22T13:34:19Z

Fix #804 .

Not sure if this contribution should be considerate a significant one.
If it's required to follow some extra procedures, please let me know.

Microsoft Contribution License Agreements (msftclas) · 2019-01-22T13:34:31Z

All CLA requirements met.

Fixes microsoft#804

Dhaval Doshi (dhaval24) · 2019-01-24T06:17:56Z

Reiley Yang (@reyang) Arthur Little (@littleaj) could you please help reviewing this contribution. In short, it is to improve our exception capturing ability by triming down the stack trace size to what is supported by backend.
vlev thanks a lot for your contribution much appreciated.

Dhaval Doshi (dhaval24) · 2019-01-24T15:13:04Z

vlev a quick comment on naming first. I would rather prefer something different than traceLength. trace is common terminology in Distributed Tracing (a.k.a correlation) and also is sometimes intermixed as logs. This would create yet another confusing.

May be exceptionTraceLength or something of that sort is better alternative. Reiley Yang (@reyang) your thoughts on this?

PS: Hardcoding is not what I would prefer. Instead, customer configuration is a good choice. However, it still wont solve all the problems and some packets mights still drop because customer misconfigured it. In this case, it might be wise to log a message for troubleshooting when the backend responds with partial accept.

Dhaval Doshi (dhaval24) · 2019-01-24T15:17:11Z

 public final class DataOfConfigurationForException {
    private boolean enabled = false;
-    private int stackSize = Integer.MAX_VALUE;
+    private Integer maxStackSize;


why do we need Class type Integer instead of primitive type int?

The idea is to avoid the duplication of decision logic for default values (in xml, sb and agent config). It seems more convenient to keep it in one place (see ExceptionTelemetryOptions.java).
Null value, in case of the absence of specific configuration, seems like a natural choice.

Dhaval Doshi (dhaval24) · 2019-01-24T15:17:52Z


-    public int getStackSize() {
-        return stackSize;
+    public Integer getMaxStackSize() {


can this method be converted to return primitive int instead of Boxed type Integer. I do not see any specific need of it.

The idea is to avoid the duplication of decision logic for default values (in xml, sb and agent config). It seems more convenient to keep it in one place (see ExceptionTelemetryOptions.java).
Null value, in case of the absence of specific configuration, seems like a natural choice.

Dhaval Doshi (dhaval24) · 2019-01-24T15:22:30Z

+        ApplicationInsightsProperties.Exceptions exceptionsConfig = applicationInsightsProperties.getExceptions();
+        if (exceptionsConfig != null) {
+            Integer maxStackSize = exceptionsConfig.getMaxStackSize();
+            if (maxStackSize != null) {


Put a diagnostic log to indicate that default value will be picked for this which is Integer max when it's left null

Dhaval Doshi (dhaval24) · 2019-01-24T15:22:59Z

+                telemetryConfiguration.setMaxStackSize(maxStackSize);
+            }
+            Integer maxTraceLength = exceptionsConfig.getMaxTraceLength();
+            if (maxTraceLength != null) {


When this is null, I think default value will be set to Integer Max. We should have a diagnostic log here to indicate that.

Dhaval Doshi (dhaval24) · 2019-01-24T15:27:16Z

     * @param metrics Measurements associated with this exception event. Appear in "custom metrics" in Metrics Explorer.
     */
-    public void trackException(Exception exception, Map<String, String> properties, Map<String, Double> metrics) {
+    public void trackException(Throwable exception, Map<String, String> properties,


It's a good move to make this signature to accept Throwable. I think this is also not a breaking change.

Dhaval Doshi (dhaval24) · 2019-01-24T15:31:36Z

+    private int maxStackSize;
+    private int maxTraceLength;
+
+    public ExceptionTelemetryOptions(Integer maxStackSize, Integer maxTraceLength) {


I think it's good to comment out that default values would be Integer Max for future readers.

I've added the description of default values into class level javadoc.

Dhaval Doshi (dhaval24) · 2019-01-24T15:34:43Z

            telemetryClient.track(telemetry);
            if (throwable != null) {
                InternalLogger.INSTANCE.trace("Sending Sql exception");
-                ExceptionTelemetry exceptionTelemetry = new ExceptionTelemetry(throwable);


Nice move to consolidate this logic.

Dhaval Doshi (dhaval24) · 2019-01-24T16:07:43Z

-     * @param stackSize The max stack size to report.
-     * @param exception The exception to track.
-     */
-    public ExceptionTelemetry(Throwable exception, int stackSize) {


Are you removing this public constructor? In that case it might be a breaking change, though rarely anyone is relying on it.

I've reverted this change.

Dhaval Doshi (dhaval24) · 2019-01-24T16:46:58Z

-        ExceptionTelemetry exceptionTelemetry = new ExceptionTelemetry(exception);
-
-        NullPointerException exception1 = new NullPointerException("mock");
-        exceptionTelemetry.setException(exception1);


It seems that you have removed this setter in ExceptionTelemetry class. It was a public method and therefore it means it could be a breaking change. I would like to avoid any breaking change here.

I've reverted this change.

Dhaval Doshi (dhaval24)

The design seems to be good, however naming, and some breaking changes needs to be addressed at the very least I think.

Dhaval Doshi (dhaval24) · 2019-01-25T05:41:56Z

vlev to give you some more context, we need to think a bit about this design overall. It might be a good idea to have these properties configurable, however it still leaves a gap for users to make this call and it can well end up that they might misconfigure these properties and hence the telemetry doesn't end up in backend.

We also need to ensure that the problem is solved ubiquitously across languages. For example I am not sure there is a similar approach taken in C# or other Application Insights SDK. Sergey Kanzhelev (@SergeyKanzhelev) what are your thoughts and how does C# handle this?

We need to understand a bit deeply wether we needed adjustments in our backend to handle large exceptions stack traces properly, wether we need a backend adjustment. or if we need to do it on SDK side we need to ensure design is consistent across SDKs.

I would put the PR on a small hold so we can have a better design discussion here on Github. vlev much appreciate that you brought light to this topic again.

Fixes microsoft#804 traceLength renamed into exceptionTraceLength

Fixes microsoft#804 - Logging of exception handling configuration - Backward incompatible changes in ExceptionTelemetry were reverted

vlev · 2019-01-28T16:32:17Z

The design seems to be good, however naming, and some breaking changes needs to be addressed at the very least I think.

I've reverted all breaking changes and renamed traceLength to exceptionTraceLength.

vlev · 2019-01-28T16:45:08Z

vlev to give you some more context, we need to think a bit about this design overall. It might be a good idea to have these properties configurable, however it still leaves a gap for users to make this call and it can well end up that they might misconfigure these properties and hence the telemetry doesn't end up in backend.

Well, I agree - this extra configuration can not be treated as a final solution to #482.
Nevertheless, it could provide some control and confidence in AI tracing capabilities. Also, this configuration might be useful for those who want to apply some constrains on the amount of data being sent and stored in AI backend. For instance, some legacy applications might generate quite a big amount of white noise. Which could be reflected in storage account bills.

Dhaval Doshi (dhaval24) · 2019-01-29T04:49:14Z

Also, this configuration might be useful for those who want to apply some constrains on the amount of data being sent and stored in AI backend.

Regarding this, yes it is a perfectly valid scenario. However, we generally recommend users to implement their own class which implements TelemetryProcessor interface and you can then write your custom logic to trim the stacktrace. It might provide more flexibility for users to choose their own way of trimming exceptions. May be they want to trim stacktrace from top or bottom or take parts of top and bottom and trim in the middle.

I am fine with either decision, but to make this solution a generic solution, we need to have consistency across different AI SDKs so that we do not confuse users, to adopt to one solution in one language and another solution in another.

I know this might sound a bit stringent, but once a configuration change is introduced it remains in the users configuration file for decade and it's very hard to revert it. So I do want to proceed with caution here.

vlev · 2019-02-21T08:18:11Z

Dhaval Doshi (@dhaval24)
Could you please tell, if there are any updates on this issue?
Do you guys have some timeline in mind?

Or, maybe, you need some additional input from my side?

Dhaval Doshi (dhaval24) · 2019-02-23T00:15:41Z

Sergey Kanzhelev (@SergeyKanzhelev) do we face similar issue in .NET SDK? If yes, how did we resolved that? If not, what are your thoughts on above approach from vlev

vlev · 2019-05-06T06:43:20Z

Sergey Kanzhelev (@SergeyKanzhelev), Dhaval Doshi (@dhaval24) Any news on the issue?

Marcin Bazydlo (gondar) · 2019-05-06T15:43:28Z

Sergey Kanzhelev (@SergeyKanzhelev) do we know if similar issue is problem for .Net SDK?

Current behaviour in Java is very worrying and basically means AppInsights is useless as centralised logs. What's the point of having those when I cannot be sure I have all exceptions there?

We already had 2 cases in our recent production when important issue was missed (our application happens to have huge stack traces).

Dhaval Doshi (@dhaval24) do we have any examples of how we should implement our own TelemetryProcessor to fix the issue above?

Dhaval Doshi (dhaval24) · 2019-05-06T16:57:56Z

Trask Stalnaker (@trask) FYI - this issue persist in our SDK ( where huge stack traces are not sent due to schema limits). This is a design question in general and Reiley Yang (@reyang) (If I remember correctly) also had some thoughts about it.

It depends on the type of application. Assuming it is a springboot type application you can easily wire up a processor like this:

@Bean
    public TelemetryProcessor myProcessor() {
        return (Telemetry t) -> {
            if (t instanceof ExceptionTelemetry) {
                // do some manipulation of stacktrace ...
            }
            // always return true, as there is no filtering.
            return true;
        };
    }

Springboot application will then wire up this processor. If it's java ee app similar logic exist.

Arthur Little (@littleaj) / Trask Stalnaker (@trask) can assist more on it.

Marcin Bazydlo (gondar) · 2019-05-07T21:54:49Z

Thanks,
I sort of know the TelemetryProcessor syntax. But getting telemetry smaller was a bit trickier. Below my implementation:

public static final int MAX_STACK_LENGTH = 100;
public static final int MAX_INNER_EXCEPTIONS = 10;
public static final int MAX_INNER_EXCEPTION_STACK_LENGTH = 10;
@Override
public boolean process(Telemetry telemetry) {
    if (telemetry instanceof ExceptionTelemetry) {
        Throwable throwable = ((ExceptionTelemetry) telemetry).getThrowable();
        cutStackTraceIfNeeded(throwable, MAX_STACK_LENGTH);
        ((ExceptionTelemetry) telemetry).setException(throwable, MAX_INNER_EXCEPTIONS);
    }
    return true;
}

private void cutStackTraceIfNeeded(Throwable throwable, int maxStackLength) {
    if (throwable == null)
        return;
    StackTraceElement[] stack = throwable.getStackTrace();
    if (stack.length > maxStackLength) {
        StackTraceElement[] throttledStack = Arrays.copyOf(stack, maxStackLength);
        throwable.setStackTrace(throttledStack);
    }
    cutStackTraceIfNeeded(throwable.getCause(), MAX_INNER_EXCEPTION_STACK_LENGTH);
}

Let me know if you have some ideas of what I could possibly miss. What I dislike about this implementation the most is that transforming of throwable stackTrace happens twice: first in logsappender and than in my processor. That's going to be performance hit ;-/.

vlev · 2019-08-08T06:12:21Z

Dhaval Doshi (@dhaval24)
Should I cancel this pull request?

Looks like you've decided to stick with the workaround.
Could you please include this into the documentation? Each of the projects using AI would need to include it into their source code.

Trask Stalnaker (trask) · 2022-08-15T21:34:02Z

closing, addressed by #1702

XML/SB/Agent configuration for exceptions truncation

f02acff

Fixes microsoft#804

vlev force-pushed the master branch from 7873b02 to f02acff Compare January 23, 2019 07:21

Dhaval Doshi (dhaval24) requested review from Dhaval Doshi (dhaval24), Arthur Little (littleaj) and Reiley Yang (reyang) January 24, 2019 06:16

Dhaval Doshi (dhaval24) assigned vlev Jan 24, 2019

Dhaval Doshi (dhaval24) added the Enhancement label Jan 24, 2019

Dhaval Doshi (dhaval24) reviewed Jan 24, 2019

View reviewed changes

Dhaval Doshi (dhaval24) suggested changes Jan 24, 2019

View reviewed changes

vlev added 2 commits January 28, 2019 17:00

XML/SB/Agent configuration for exceptions truncation

8220c15

Fixes microsoft#804 traceLength renamed into exceptionTraceLength

XML/SB/Agent configuration for exceptions truncation

0eb72b4

Fixes microsoft#804 - Logging of exception handling configuration - Backward incompatible changes in ExceptionTelemetry were reverted

Dhaval Doshi (dhaval24) requested a review from Sergey Kanzhelev (SergeyKanzhelev) January 29, 2019 04:49

vlev removed their assignment Aug 9, 2019

vlev requested review from Helen (heyams) and Trask Stalnaker (trask) as code owners April 26, 2022 23:42

Trask Stalnaker (trask) force-pushed the main branch from bbf495e to 51fe9c7 Compare April 26, 2022 23:44

Trask Stalnaker (trask) closed this Aug 15, 2022

Conversation

vlev commented Jan 22, 2019

Uh oh!

Microsoft Contribution License Agreements (msftclas) commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dhaval Doshi (dhaval24) commented Jan 24, 2019

Uh oh!

Dhaval Doshi (dhaval24) commented Jan 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dhaval Doshi (dhaval24) Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dhaval Doshi (dhaval24) Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dhaval Doshi (dhaval24) left a comment

Choose a reason for hiding this comment

Uh oh!

Dhaval Doshi (dhaval24) commented Jan 25, 2019

Uh oh!

vlev commented Jan 28, 2019

Uh oh!

vlev commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dhaval Doshi (dhaval24) commented Jan 29, 2019

Uh oh!

vlev commented Feb 21, 2019

Uh oh!

Dhaval Doshi (dhaval24) commented Feb 23, 2019

Uh oh!

vlev commented May 6, 2019

Uh oh!

Marcin Bazydlo (gondar) commented May 6, 2019

Uh oh!

Dhaval Doshi (dhaval24) commented May 6, 2019

Uh oh!

Marcin Bazydlo (gondar) commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vlev commented Aug 8, 2019

Uh oh!

Trask Stalnaker (trask) commented Aug 15, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Microsoft Contribution License Agreements (msftclas) commented Jan 22, 2019 •

edited

Loading

Dhaval Doshi (dhaval24) Jan 24, 2019 •

edited

Loading

Dhaval Doshi (dhaval24) Jan 24, 2019 •

edited

Loading

vlev commented Jan 28, 2019 •

edited

Loading

Marcin Bazydlo (gondar) commented May 7, 2019 •

edited

Loading