Support for Activity Status and StatusDescription via switch #2605
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2605 +/- ##
==========================================
+ Coverage 81.31% 81.37% +0.05%
==========================================
Files 245 247 +2
Lines 8645 8686 +41
==========================================
+ Hits 7030 7068 +38
- Misses 1615 1618 +3
|
| /// </summary> | ||
| /// <param name="tracerProviderBuilder">TracerProviderBuilder instance.</param> | ||
| /// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns> | ||
| public static TracerProviderBuilder DisableActivityStatusSwitch(this TracerProviderBuilder tracerProviderBuilder) |
There was a problem hiding this comment.
Since the switch is static (global), we might want to just expose it statically. Kind of like we do for the global propagator. I think having these extensions on TracerProviderBuilder might mislead users into thinking the setting is per-tracer.
| /// If true then activity Status and StatusDescription properties will be set | ||
| /// using tags otel.status_code and otel.status_description respectively. | ||
| /// </summary> | ||
| public static bool ActivityStatusSwitch { get; set; } = true; |
There was a problem hiding this comment.
Trying to think of a more descriptive name. How about "StatusTagMigrationEnabled"?
I think for the comments something like...
/// <summary>
/// Gets or sets a value indicating whether or not activity status migration is enabled. Default value: true.
/// </summary>
/// <remarks>
/// If true then <see cref="Activity.Status"/> and <see cref="Activity.StatusDescription"/> properties (added in .NET 6) will be set
/// from `otel.status_code` and `otel.status_description` tag values respectively prior to export. For more details see <a href="migration_doc_url_here">migration doc name here</a>.
/// </remarks>There was a problem hiding this comment.
Done - let me know if you have a suggestion for class name as well :)
vishweshbankwar
changed the title
vishweshbankwar
changed the title
vishweshbankwar
changed the title
| @@ -47,6 +48,16 @@ public sealed override void OnStart(T data) | |||
|
|
|||
| public override void OnEnd(T data) | |||
| { | |||
| var activity = data as Activity; | |||
There was a problem hiding this comment.
this will add unwanted perf impact, when using LogRecord.
I think we need to add BaseActivityExportProcessor, which inherits BaseExportProcessor<Activity>.
There was a problem hiding this comment.
It will be same as SimpleActivityExportProcessor and BatchActivityExportProcessor right?
The reason we wanted to put it here is to cover cases where exporters are inheriting from BaseExportProcessor and not overriding OnEnd.
Instead of introducing new one - should we add this check in Batch and Simple Activity processor (this is what I had before).
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes 2. under proposal in #2569
Todo:
Doc update.
Update #2572 to consider this change.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes