opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

Add unified enrichment support for Log/Trace/Metric

Open evgenyfedorov2 opened this issue 2 years ago • 8 comments

Feature Request

Is your feature request related to a problem?

Enriching telemetry with addition of useful information is essential for most of the users. Today OpenTelemetry .NET library provides ability to users for enriching collected telemetry. However, the mechanisms to enrich logs, traces and metrics differs greatly. Today if a customer onboards to OpenTlemetry .NET for logs, traces and metrics then they need to learn and maintain 3 different mechanisms for enrichment. The lack of a unified mechanism to enrich logs, traces and metrics ends up hurting the developers. Another point to note is that DI is quite ingrained in modern .NET services and lack of DI based mechanism to enrich also results in increased effort on the developers.

Describe the solution you'd like:

The proposal here is to introduce a unified enrichment framework that works in a consistent way to enrich all telemetry. We have common libraries built on top of current OpenTelemetry .NET SDK for solve the telemetry need for services within multiple orgs in Microsoft. Enrichment is one of the essential capability that all these services need and we have an consitent enrichment framework that's been quite successful.

We are proposing the unified enrichment model to OpenTelemetry .NET so everyone can benefit and there are better possibilities for writing open telemetry extensions and achive better performance for enrichment.

Expose the following base abstract class for Enrichment:

public abstract class BaseEnricher<T>
{
    public virtual void Enrich(T enrichmentBag)
    { }
}

Extensions to register enricher:

public IServiceCollection AddLogEnricher<T>(IServiceCollection services) : where T: class, LogEnricher {}
public IServiceCollection AddTraceEnricher<T>(IServiceCollection services) : where T: class, TraceEnricher {}
public IServiceCollection AddMetricEnricher<T>(IServiceCollection services) : where T: class, MetricEnricher {}

LogEnricher, TraceEnricher, MetricEnricher are defined as

public abstract class LogEnricher : BaseEnricher<LogEnrichmentBag> {}
public abstract class TraceEnricher : BaseEnricher<TraceEnrichmentBag> {}
public abstract class MetricEnricher : BaseEnricher<MetricEnrichmentBag> {}

LogEnrichmentBag, TraceEnrichmentBag, MetricEnrichmentBag are derived from the base enrichment bag

public abstract class EnrichmentBag
{
    public void Add(string key, object? value);
}
public abstract class LogEnrichmentBag : EnrichmentBag {}
public abstract class TraceEnrichmentBag : EnrichmentBag {}
public abstract class MetricEnrichmentBag : EnrichmentBag {}

Usage

A developer can then write and register enrichers for logs, traces and metrics in consistent manner, i.e. write as many enrichers as you need:

public class MyLogEnricher : LogEnricher
{
    public void Enrich(LogEnrichmentBag enrichmentBag)
    {...}
}

public class MyTraceEnricher : LogEnricher
{
    public void Enrich(LogEnrichmentBag enrichmentBag)
    {...}
}

public class MyFirstMetricEnricher : LogEnricher
{
    public void Enrich(LogEnrichmentBag enrichmentBag)
    {...}
}

public class MySecondMetricEnricher : LogEnricher
{
    public void Enrich(LogEnrichmentBag enrichmentBag)
    {...}
}

// Register all required enrichers
services.AddLogEnricher<MyLogEnricher>();
services.AddTraceEnricher<MyTraceEnricher>();
services.AddMetricEnricher<MyFirstMetricEnricher>();
services.AddMetricEnricher<MySecondMetricEnricher>();

Similarly the same model is then used for enrichment for instrumentation libraries e.g. AspNetCore instrumentation library will expose the following abstract class using the same enrichment bags

public abstract class AspNetCoreInstrumentationEnricher<T>
{
  public virtual void EnrichWithHttpRequest(T enrichmentBag, HttpRequest request) { }
  public virtual void EnrichWithHttpResponse(T enrichmentBag, HttpResponse response) { }
  public virtual void EnrichWithException(T enrichmentBag, Exception exception) { }
}

public class LogAspNetCoreInstrumentationEnricher : AspNetCoreInstrumentationEnricher<LogEnrichmentBag> {}
public class MetricAspNetCoreInstrumentationEnricher : AspNetCoreInstrumentationEnricher<MetricEnrichmentBag> {}
public class TraceAspNetCoreInstrumentationEnricher : AspNetCoreInstrumentationEnricher<TraceEnrichmentBag> {}

// Write and register enrichers specific to asp net core instrumentation
services.AddAspNetCoreInstrumentationLogEnricher<MyIncomingHttpRequestLogEnricher>();
services.AddAspNetCoreInstrumentationMetricEnricher<MyIncomingHttpRequestMetricEnricher>();

Describe alternatives you've considered.

One modification to the above design is to remove the Log/Trace/Metric specific classes and simplify it to

public class EnrichmentBag
{
    public void Add(string key, object? value);
}

public abstract class Enricher
{
    public abstract EnricherType Type {get;} // EnricherType is an enum with Log, Trace and Metric as enum values
    public void Enrich(EnrichmentBag enrichmentBag) {}
}

public static IServiceCollection AddEnricher<T>(this IServiceCollection services) : where T: class, Enricher; 
// Developers can then write their enrichers public class MyLogEnricher : Enricher
{
    public EnricherType Type => EnricherType.Log;
    public void Enrich(EnrichmentBag enrichmentBag){}
}

services.AddEnricher<MyLogEnricher>();

While this is a simplified design it has a few cons as compared to the above proposed version

  • When reading code a developer can't really know whether the enricher registerd is a log, trace or a metric enricher. They need to look at the enricher class to find that out. Not a big issue at the cost of simplification.
  • For implementing the logic that uses the enrichers, DI will give you all enrichers and then logic needs to be added to extract out the desired (log, trace or metric) enrichers before using it.

Additional Context

Add any other context about the feature request here.

evgenyfedorov2 avatar Jan 23 '23 17:01 evgenyfedorov2

@reyang @CodeBlanch

dpk83 avatar Jan 23 '23 17:01 dpk83

@open-telemetry/dotnet-maintainers

I think this is a good set of features after the 1.4.0 release. I don't feel OpenTelemetry.dll is the right place, and would recommend opentelemetry-dotnet-contrib as the repo (maybe OpenTelemetry.Extensions.Enrichment.dll), because of the following considerations:

  1. In general OpenTelemetry.dll is trying to follow the SDK specifications under https://github.com/open-telemetry/opentelemetry-specification, the goal is not to provide many convenient layers or integration with frameworks from OpenTelemetry.dll, extensions should be used instead (e.g. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Extensions.Hosting and https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Extensions.PersistentStorage).
  2. We probably would want to keep OpenTelemetry.dll small - from the binary size perspective and the package depencencies - this will be important when it comes to mobile/client scenarios.
  3. From engineering perspective - keeping this as a separate package will help to control the publicly exposed API surface from OpenTelemetry.dll, which helps to decouple the engineering/release cycle.

reyang avatar Jan 23 '23 23:01 reyang

Yeah, we can start with getting this into opentelemetry-dotnet-contrib repo. We can get started working on the feature now and can get this in after 1.4.0 release.

Shall we just start an initial draft PR in the contrib repo so we can get early feedback and so it's clear on how everything will compose together?

dpk83 avatar Jan 24 '23 17:01 dpk83

Shall we just start an initial draft PR in the contrib repo so we can get early feedback and so it's clear on how everything will compose together?

Check these:

reyang avatar Jan 24 '23 17:01 reyang

@evgenyfedorov2 @dpk83 My recommendation would be to pick one signal to start with. If it was me, I would do tracing. Just knowing how the SDK works and the DI surface, I think you could build this as an add-on successfully. Metrics you could get most of the way there but I think you'll run into some modification to SDK being needed. Logs maybe wait a bit. See my comment here. We are anticipating making some changes soon which will bring logs up to parity with tracing & metrics.

Some comments about the proposed API...

  • Could we use TagList instead of EnrichmentBag?

  • It seems using the API you may only add tags during enrichment. The APIs we have today are more flexible IIRC. You get the DTO and the "context" thing and you are free to do whatever to the DTO. Would it make sense to have more flexibility?

CodeBlanch avatar Jan 24 '23 18:01 CodeBlanch

My recommendation would be to pick one signal to start with. If it was me, I would do tracing.

@CodeBlanch Makes sense. For metric enrichment we will need to wait for the ResourceProvider support in Geneva exporter and Tracing seems a good one to get started with.

Could we use TagList instead of EnrichmentBag?

While TagList helps avoid allocation it's only if the total dimensions are less than 8 while we would like to have a greater limit. Having the EnrichmentBag will give us more control both for future expansions and avoiding allocation for more than 8 keys by utilizing pools.

It seems using the API you may only add tags during enrichment. The APIs we have today are more flexible IIRC. You get the DTO and the "context" thing and you are free to do whatever to the DTO. Would it make sense to have more flexibility?

Yes the enrichment is specifically scoped for enrichment purpose. The LogEnrichmentBag, TraceEnrichmentBag and MetricEnrichmentBag also provide this flexibility where the specific enrichment bag will carry the context. i.e. TraceEnrichmentBag can expose Activity object and MetricEnrichmentBag can carry the Instrument object etc. Again that depends on whether we want to provide that flexibility.

dpk83 avatar Jan 24 '23 18:01 dpk83

This issue was marked stale due to lack of activity and will be closed in 7 days. Commenting will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions[bot] avatar Sep 25 '24 03:09 github-actions[bot]

Closed as inactive. Feel free to reopen if this issue is still a concern.

github-actions[bot] avatar Oct 03 '24 03:10 github-actions[bot]