apm-agent-dotnet icon indicating copy to clipboard operation
apm-agent-dotnet copied to clipboard

No way to set `Code` property on captured exception

Open vhatsura opened this issue 4 years ago • 4 comments

Is your feature request related to a problem? Please describe.

According to the specification, code field is the error code set when the error happened, e.g. database error code. Not all databases and drivers return the error code when error occurred, however, in the case of MongoDB error such code presents and can be accessed from MongoCommandException. Unfortunately, in the current agent codebase is impossible to set such property.

Describe the solution you'd like

I don't have any exact suggestions right now, however, I see two possible ways how it can be achieved:

  • Extend CaptureError and CaptureException contracts to accept additional argument code. It will solve issue which is addressed here, however, in the future, it will look like hell if method contracts will be extended for every custom property.
  • Extend CaptureError and CaptureException contracts to return captured error instance. It's a more flexible way which allows in the future to also override other fields, however, at the moment it needs more work:
    • extend IError interface with additional fields
    • introduce an interface for CapturedException class

Describe alternatives you've considered

At the moment, only a reflection is an option, which is not very safe and good.

Additional context

The same is applicable for customizing Message field of the CapturedException class. MongoCommandExceptin has ErrorMessage property which contains a more detailed error message from MongoDB.

vhatsura avatar Apr 11 '20 12:04 vhatsura

update: IError interface was extended and CapturedException, CapturedStackFrame classes became public. So, the only thing is left: Extend CaptureError and CaptureException contracts to return captured error instance.

@gregkalapos, I know that it's a breaking change and all custom implementations need to change codebase, but it looks like the most flexible approach.

vhatsura avatar May 16 '20 19:05 vhatsura

Hey, @gregkalapos

I'm returning back to this project and going on my previous issues. After digging into the codebase, I know the actual reason why returning IError interface isn't a good idea. The reason in queuing such error into the payload sender and send process isn't deterministic, which means that modification of the returned IError instance can be not sent into the APM server. But another possible solution came to mind (also has some limitations at the moment). We can introduce a new method CaptureError which will accept the instance of IError interface. It will allow customizing how errors need to be looked for various dependencies. However, in such case, it would be nice to make StacktraceHelper as public. WDYT?

vhatsura avatar Oct 11 '20 09:10 vhatsura

@vhatsura - wouldn't it help to just add an optional parameter to the CaptureError and CaptureException methods? Can't you set it when the error is capture?

Also fyi this branch contains slightly related POC which extends the error capturing to automatically capture logs on error level as error (the last 3 commits on the PR are the ones implementing it). - It comes from this issue and in that case I also just added optional parameters. I know the parameter explosion is not the nicest, but very few things can go wrong with that.

gregkalapos avatar Oct 12 '20 11:10 gregkalapos

@vhatsura - wouldn't it help to just add an optional parameter to the CaptureError and CaptureException methods? Can't you set it when the error is capture?

Also fyi this branch contains slightly related POC which extends the error capturing to automatically capture logs on error level as error (the last 3 commits on the PR are the ones implementing it). - It comes from this issue and in that case I also just added optional parameters. I know the parameter explosion is not the nicest, but very few things can go wrong with that.

Optional parameters can help, but I afraid of the growing amount of parameters. Also, each new optional parameter will break binary compatibility with preserving the source compatibility.

vhatsura avatar Oct 12 '20 15:10 vhatsura