[Instrumentation.GrpcCore] Record exceptions as activity events flag

[IliaBrahinets]

Changes

RecordException flag has been added in both ServerTracingInterceptorOptions and ClientTracingInterceptorOptions. When set to true exceptions are recored as activity events.

For significant contributions please make sure you have completed the following items:

• Appropriate CHANGELOG.md updated for non-trivial changes
• Design discussion issue #
• Changes in public API reviewed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: IliaBrahinets / name: Ilia Brahinets (5560dac, 81797dd, cbd6d16, a657358, 47d38d6, 7a67c9b, cf571ff, dac6d0b, d3e6982, a728f19, 022bfda, b94db46, 5078c3b, f45da17, 81b79a2, 1c991a7, 49be52b, 5a201df, 08cfb17, 99cfaf2)
• ✅ login: cijothomas / name: Cijo Thomas (32195cf, 0d3464a, d8ef9e6)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.17%. Comparing base (71655ce) to head (0d3464a).
Report is 251 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
+ Coverage   73.91%   80.17%   +6.25%     
==========================================
  Files         267      116     -151     
  Lines        9615     3077    -6538     
==========================================
- Hits         7107     2467    -4640     
+ Misses       2508      610    -1898     

Flag	Coverage Δ
unittests-Solution	80.17% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 257 files with indirect coverage changes

[IliaBrahinets]

@cijothomas , @Kielek I've made suggested changes. Ready for another review

[Kielek]

@cijothomas, based on https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-spans.md

I would say that when possible exception should be always recorded as events. Do we really need the option to enable it? I would consider adding possibility to opt-out. To met the spec, it should be enabled by default.

@IliaBrahinets, please do not change the code based on this comment. We need to find agreement here, it should probably impact more packages than only this one.

[Kielek]

All this const are used only in tests file. I am not sure if it worth to put it into production code.

[IliaBrahinets]

Then existing AttributeActivityIdentifier const should be also taken into account. I think it is simpler to manage all semantic convetions from one place within a library (including tests). Libraries which use Shared/SemanticConventions.cs include many unused consts. BTW probably it is worth to migrate OpenTelemetry.Instrumentation.GrpcCore to Shared/SemanticConventions.cs. though it lacks "otel.status_code" and "otel.status_description" attributes.

[Kielek]

Any references to otel.status_code and otel.status_descrription should be removed from instrumentation libraries.
It should be responsibility of exporter to map it correctly. If these are still used in production code, it is just leftover from open-telemetry/opentelemetry-dotnet#2569

[IliaBrahinets]

Turns out that otel.status_code and otel.status_descrription are used in tests only. As the result of the discussion:

• otel.status_code and otel.status_description should be replaced by Activity.Status and Activity.StatusDescription in tests
• Use Shared/SemanticConventions.cs in OpenTelemetry.Instrumentation.GrpcCore

I would like to submit another PR for the suggested changes, @Kielek what do you think ?

[Kielek]

PRs are more than welcomed :)

[cijothomas]

@cijothomas, based on https://github.com/open-telemetry/semantic-conventions/blob/main/docs/exceptions/exceptions-spans.md

I would say that when possible exception should be always recorded as events. Do we really need the option to enable it? I would consider adding possibility to opt-out. To met the spec, it should be enabled by default.

@IliaBrahinets, please do not change the code based on this comment. We need to find agreement here, it should probably impact more packages than only this one.

I think it should be off by default, and controlled via an option.

1. Exceptions are expensive (string allocations, and the cost of obtaining stack etc.).
2. Exceptions could contain sensitive/pii things. (in the message usually, that is included when ToString() is invoked)
3. It is possible that the exception is already reported using other mechanism (like ILogger), so this could be just undesired duplicate information.
4. All existing instrumentation in the main repo/this repo offer this option already and they are all opt-in.
5. We have seen folks asking for on/off flag for ActivityEvent in general before. One example is here.

[Kielek]

LGTM.

@IliaBrahinets, please resolve conflicts. It is related to moving publicAPI file to the parent folder. Ref: #1711

@pcwiese, it will be great if you can review

[IliaBrahinets]

LGTM.

@IliaBrahinets, please resolve conflicts. It is related to moving publicAPI file to the parent folder. Ref: #1711

@pcwiese, it will be great if you can review

The conflicts have been resolved.