Add OTLP for Activity Exporter #679
Conversation
pjanotti
requested review from
bruno-garcia,
cijothomas,
discostu105,
lmolkova,
MikeGoldsmith and
SergeyKanzhelev
as code owners
| // and use Console exporter | ||
| OpenTelemetrySdk.EnableOpenTelemetry( | ||
| builder => builder | ||
| .AddActivitySource("Samples.SampleServer") |
There was a problem hiding this comment.
@cijothomas I'm leaning into listening to all sources by default and filter out un-wanted ones. Depending on how some libraries are instrumented we may need some processors to filter out some activities.
There was a problem hiding this comment.
Yup. Lets address this separately.
Libraries like httpclient, which was instrumented using DiagnosticSource, will have their activities emitted with empty ActivitySource. Listening to them requires AddActivitySource("") now.
Anyway, lets address this after some exporters/adapters are ready with Activity API.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs
Outdated
Show resolved
Hide resolved
| // TODO: attempt to convert to supported types? | ||
| // TODO: enforce no duplicate keys? | ||
| // TODO: drop if Value is null? | ||
| // TODO: reverse? | ||
| var attrib = new OtlpCommon.AttributeKeyValue { Key = kvp.Key, StringValue = kvp.Value }; | ||
| otlpSpan.Attributes.Add(attrib); |
There was a problem hiding this comment.
@reyang @cijothomas @noahfalk I think that this is one of the big things to have a wrapper so far: here OTel defines some behaviors that are quite different. In principle, we could have a processor to enforce the OTel spec here but that takes away the control from authors that want to enforce that spec on their code. The OTel related spec: https://github.com/open-telemetry/opentelemetry-specification/blob/bdf39cb935a6f48badc687daeb78a66dc352761f/specification/trace/api.md#set-attributes
There was a problem hiding this comment.
This is being addressed in Tarek and Noah. We can live with this now, and hopefully get better solution in next preview of DiagnosticSource.
There was a problem hiding this comment.
Agree this is a valid reason for shim/wrapper to enforce things like no duplicates etc.
| private static Dictionary<Resource, Dictionary<ActivitySource, List<OtlpTrace.Span>>> GroupByResourceAndLibrary( | ||
| IEnumerable<Activity> activityBatch) | ||
| { | ||
| // TODO: there is no Resource associated here, other SDKs associate it to the span, that's not an option here. |
There was a problem hiding this comment.
There is already an issue tracking this, see #649.
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/ActivitySourceExporter.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/ActivitySourceExporter.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs
Show resolved
Hide resolved
| @@ -79,6 +80,19 @@ public override Task<ExportResult> ExportAsync(IEnumerable<Activity> activityBat | |||
| foreach (var activityEvent in activity.Events) | |||
| { | |||
| System.Console.WriteLine($"Event Name: {activityEvent.Name} TimeStamp: {activityEvent.Timestamp}"); | |||
| foreach (var attribute in activityEvent.Attributes) | |||
pjanotti
force-pushed
the
add-activity-support-to-otlp
branch
from
f1343c3 to
9b5f3d6
Compare
* add Paulo as the approver * update CODEOWNERS * fix nit * Add OTLP for Activity Exporter (#679) * Add OTLP Exporter for Activity * Initial PR feedback * Check ID format and use DisplayName * Skip the activity instances that could not be translated Co-authored-by: Cijo Thomas <[email protected]> * Rename Adapter to Instrumentation per latest spec (#681) Co-authored-by: Mike Goldsmith <[email protected]> Co-authored-by: Paulo Janotti <[email protected]> Co-authored-by: Cijo Thomas <[email protected]> Co-authored-by: Mike Goldsmith <[email protected]>
Per our conversation, this is the Activity Exporter for OTLP consuming
Activitybatches.I opted at this time to keep
ActivityExporteras it is (there was a quick chat on Gitter on perhaps adding theExportAsync(IEnumerable<Activity>, CancellationToken)but let's have a separate discussion for it).I will be commenting on some points of the PR to some of the potential issues/follow-ups.
Checklist