Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unified enrichment support for Log/Trace/Metric #4097

Closed
evgenyfedorov2 opened this issue Jan 23, 2023 · 8 comments
Closed

Add unified enrichment support for Log/Trace/Metric #4097

evgenyfedorov2 opened this issue Jan 23, 2023 · 8 comments
Labels
enhancement New feature or request Stale Issues and pull requests which have been flagged for closing due to inactivity

Comments

@evgenyfedorov2
Copy link

evgenyfedorov2 commented Jan 23, 2023

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 evgenyfedorov2 added the enhancement New feature or request label Jan 23, 2023
@dpk83
Copy link

dpk83 commented Jan 23, 2023

@reyang @CodeBlanch

@reyang
Copy link
Member

reyang commented Jan 23, 2023

@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.

@dpk83
Copy link

dpk83 commented Jan 24, 2023

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?

@reyang
Copy link
Member

reyang commented Jan 24, 2023

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:

@CodeBlanch
Copy link
Member

@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?

@dpk83
Copy link

dpk83 commented Jan 24, 2023

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.

Copy link
Contributor

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 github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 25, 2024
Copy link
Contributor

github-actions bot commented Oct 3, 2024

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

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

No branches or pull requests

4 participants