EnrichmentScope & EnrichingActivityProcessor #969

CodeBlanch · 2020-08-01T16:59:56Z

Update: Released on NuGet here: https://www.nuget.org/packages/Macross.OpenTelemetry.Extensions

Relates to #73. Relates to #809 (option 3, StartAction).

Changes

Added the concept of an enrichment scope. The idea is to give users the ability to add contextual data to the Activity created performing some operation, in a global way.

examples/AspNetCore/Controllers/WeatherForecastController.cs

reyang · 2020-08-01T18:31:16Z

src/OpenTelemetry/Trace/EnrichmentScope.cs

+    /// <summary>
+    /// A class for enriching the data of <see cref="Activity"/> objects.
+    /// </summary>
+    public class EnrichmentScope : IDisposable


I know I'm always bad at naming things, the name ContextualProcessor keeps popping up in my mind 😂

(I'm not opposed to changing the name. I was just waiting to see if anyone else thought similar or had other ideas.)

Should be my last question in this PR: do we envision a similar concept would be introduced for metrics/logs in the future? If yes, we might need to consider how we name things so we won't be cornered in the future.

This is an interesting point. Slight tangent, but my mental model of "instrumentation" before OTel has been one that combines the concepts of collecting of traces, metrics, etc together. High-level I think of something like HTTP instrumentation as a thing that both generates spans in a trace as well as collecting metrics about the operation. From a configuration perspective, I've wondered if these things will continue to be treated as separate - that is, I build up my trace pipeline/configuration separate from my metric configuration.

In the context of EnrichmentScope, should there be a different trace vs. metric vs. log EnrichmentScope concept? Or can all of these be combined in one EnrichmentScope class that includes the ability to configure behavior on traces, metrics, and logs?

There's nothing really Activity-specific about the current implementation. It uses @reyang's RuntimeContext stuff. Essentially what it is doing is buffering a callback/closure to be executed when the final thing to be enriched is available. So in that way, it could work for metrics & logs. Maybe you will be able to give it different callbacks depending on what is being enriched? I don't have enough of a mental picture of the metrics & logging APIs to say conclusively, but it would be great if we had one thing for all the spec areas. Open to suggestions.

My suggestion is that we get this PR merged and continue the discussion here 😆

Yes, I'm leaning towards thinking one thing for all the spec areas make sense, though like you my mental picture is still somewhat fuzzy.

This is why I was pondering this thought in the realm of configuration since that's a little more concrete at the moment. For example, I can see the AddOpenTelemetry extension method on IServiceCollection include additional parameters for a "MeterProviderBuilder" and a "LogProviderBuilder" like how you're suggesting distinct callbacks based on type of telemetry data for EnrichmentScope.

My suggestion is that we get this PR merged and continue the discussion here 😆

yes! agreed.

src/OpenTelemetry/Sdk.cs

CodeBlanch · 2020-08-15T02:20:25Z

Updated the design:

Added EnrichingActivityProcessor and moved the logic to apply EnrichmentScopes into there. This was a result of @reyang's feedback. Users not interested in the enrichment no longer need to pay for the perf.
Added EnrichmentScopeTarget. This was a result of @alanwest's feedback about options for how the scope should be applied. A little tricky to implement, but I think I got it to work using a doubly-linked-list design.

Fire away!

codecov · 2020-08-15T02:21:14Z

Codecov Report

Merging #969 into master will increase coverage by 0.13%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #969      +/-   ##
==========================================
+ Coverage   75.83%   75.96%   +0.12%     
==========================================
  Files         225      227       +2     
  Lines        6225     6267      +42     
==========================================
+ Hits         4721     4761      +40     
- Misses       1504     1506       +2

Impacted Files	Coverage Δ
.../OpenTelemetry/Trace/EnrichingActivityProcessor.cs	`75.00% <75.00%> (ø)`
...tp/Implementation/HttpHandlerDiagnosticListener.cs	`88.00% <100.00%> (+0.50%)`	⬆️
src/OpenTelemetry/Trace/EnrichmentScope.cs	`100.00% <100.00%> (ø)`
...us/Implementation/PrometheusExporterEventSource.cs	`72.72% <0.00%> (+9.09%)`	⬆️

src/OpenTelemetry/Trace/EnrichmentScope.cs

reyang

LGTM.

docs/trace/enriching-activity-processor/README.md

Co-authored-by: Reiley Yang <reyang@microsoft.com>

docs/trace/enriching-activity-processor/README.md

eddynaka · 2020-08-15T23:08:32Z

@eddynaka @reyang Markdownlint is complaining about table rows being too long. How do I make that happy? Doesn't seem to be a way to break in the middle of a row.

don't know if you solved already. but you can try adding a
in the row to break and see if it complains.

docs/trace/enriching-activity-processor/README.md

CodeBlanch · 2020-08-15T23:25:46Z

@eddynaka

don't know if you solved already. but you can try adding a
in the row to break and see if it complains.

Add a what? 😄 I tried a few things, couldn't get it to work.

Here's the source:

| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will be enriched and then the scope will automatically be closed. |

Yields:

Name	Description
FirstChild	The first child `Activity` created under the scope will be enriched and then the scope will automatically be closed.

| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will
 be enriched and then the scope will automatically be closed. |

Yields:

Name	Description
FirstChild	The first child `Activity` created under the scope will
be enriched and then the scope will automatically be closed.

| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will |
|            | be enriched and then the scope will automatically be closed. |

Yields:

Name	Description
FirstChild	The first child `Activity` created under the scope will
	be enriched and then the scope will automatically be closed.

I think we may need to alter the markdownlint config to ignore tables?

eddynaka · 2020-08-15T23:27:14Z

@eddynaka

don't know if you solved already. but you can try adding a
in the row to break and see if it complains.

Add a what? 😄 I tried a few things, couldn't get it to work.

Here's the source:
| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will be enriched and then the scope will automatically be closed. |
Yields:

Name Description
FirstChild The first child Activity created under the scope will be enriched and then the scope will automatically be closed.
| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will
 be enriched and then the scope will automatically be closed. |
Yields:

Name Description
FirstChild The first child Activity created under the scope will
be enriched and then the scope will automatically be closed.
| Name | Description |
| --- | --- |
| FirstChild | The first child `Activity` created under the scope will |
|            | be enriched and then the scope will automatically be closed. |
Yields:

Name Description
FirstChild The first child Activity created under the scope will
be enriched and then the scope will automatically be closed.
I think we may need to alter the markdownlint config to ignore tables?

I sent the tag in gitter...github replaced the tag with the new line LOL

reyang

LGTM, left some minor non-blocking comments.

reyang · 2020-08-15T23:31:38Z

@eddynaka @reyang Markdownlint is complaining about table rows being too long. How do I make that happy? Doesn't seem to be a way to break in the middle of a row.

Remove the table, use bullet points. In general markdownlint tries to discourage people from putting long statements in a table due to the readability issue.

alanwest · 2020-08-17T18:37:40Z

docs/trace/enriching-activity-processor/README.md

+**Note:** Order is important. Make sure your `EnrichingActivityProcessor` is
+added before your `Exporter` and any other `ActivityProcessor`s you are using.


Not suggesting any action for this PR, but food for thought...

This note makes me wonder if we should consider codifying processor/exporter ordering in an effort to make this less prone to messing up. I don't have a clear suggestion at the moment, but maybe a combination of "type" and "priority".

Like if we know it's an "exporter type" versus "processor type", can the builder be smart enough to make sure to place it at the end?

Furthermore, maybe not all processors are equal. Some may be "high priority" and should therefore be injected at the beginning of the pipeline. This would in turn require some logic to deal with ties.

@reyang Tagging you just in case you didn't see this. Some food for thought as we improve/refactor the build-up. The ordering problem is similar to ASP.NET Core Middleware. Not much provided there, other than guidance that order is critical.

Interesting ideas about distinguishing "exporting processors" versus "enriching processors" versus other types.

Reminded me of TSR and IRP.

alanwest · 2020-08-17T19:09:00Z

docs/trace/enriching-activity-processor/README.md

+using EnrichmentScope.Begin(
+    target: EnrichmentScopeTarget.AllChildren,
+    enrichmentAction: a => a.AddTag("mycompany.customer_id", 5678))
+{


Cool, I like what is demonstrated here!

alanwest · 2020-08-17T19:21:06Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs

@@ -90,6 +90,7 @@ public override void OnStartActivity(Activity activity, object payload)

            activity.SetKind(ActivityKind.Client);
            activity.DisplayName = HttpTagHelper.GetOperationNameForHttpMethod(request.Method);
+            activity.SetCustomProperty("HttpHandler.Request", request);


How does this relate to the enrichment stuff?

In the demo I wanted to enrich by some context stuff and by adding userAgent from request & contentType from response. Surprised to notice that the raw objects were not available (they are for .NET Framework). So this is making the raw objects available to anyone downstream that might be interested.

alanwest

I'm as excited about this PR as an Enrichard Simmons!

CodeBlanch · 2020-08-19T01:17:44Z

Had a chat with @cijothomas. I'm going to take a stab at supporting this via CorrelationContext/Baggage.

CodeBlanch · 2021-03-29T05:31:38Z

If anyone lands here looking for a way to make this work I have an updated version (for 1.0) here: https://www.nuget.org/packages/Macross.OpenTelemetry.Extensions

Added EnrichmentScope.

73cee9b

CodeBlanch commented Aug 1, 2020

View reviewed changes

examples/AspNetCore/Controllers/WeatherForecastController.cs Outdated Show resolved Hide resolved

reyang reviewed Aug 1, 2020

View reviewed changes

Tweak EnrichmentScope so it doesn't flow to children where it shouldn't.

66e1724

CodeBlanch mentioned this pull request Aug 3, 2020

Suppress instrumentation upon calling ActivityExporter.ExportAsync #977

Closed

reyang reviewed Aug 14, 2020

View reviewed changes

src/OpenTelemetry/Sdk.cs Outdated Show resolved Hide resolved

CodeBlanch added 2 commits August 14, 2020 18:30

Merging from master.

d52bec5

Incorporating feedback.

35a1a9c

CodeBlanch changed the title ~~For consideration: EnrichmentScope~~ For consideration: EnrichmentScope & EnrichingActivityProcessor Aug 15, 2020

Fixed potential null ref exception.

b4d6532

reyang reviewed Aug 15, 2020

View reviewed changes

src/OpenTelemetry/Trace/EnrichmentScope.cs Show resolved Hide resolved

OnEnd isn't called unless activity.IsAllDataRequested is already true.

1b16067

reyang reviewed Aug 15, 2020

View reviewed changes

src/OpenTelemetry/Trace/EnrichmentScope.cs Outdated Show resolved Hide resolved

reyang approved these changes Aug 15, 2020

View reviewed changes

CodeBlanch added 11 commits August 14, 2020 19:38

Code review.

320c946

Parent null check.

f33057c

Bug fixes and the first round of tests.

651b4c7

Updated changelog.

04a86dd

Merge branch 'master' into enrichmentscope

e682a6a

Test artifact cleanup.

6b88c9f

Out of order disposal tests.

7eeaf58

Added log message for exception thrown enriching activity.

7f76856

Fixing spots broken by namespace change.

793a98c

Removed example code.

96f9d30

Renamed "NextActivity" -> "FirstChild". Added an example + README.

60448bd

CodeBlanch changed the title ~~For consideration: EnrichmentScope & EnrichingActivityProcessor~~ EnrichmentScope & EnrichingActivityProcessor Aug 15, 2020

CodeBlanch marked this pull request as ready for review August 15, 2020 22:50

CodeBlanch requested a review from a team August 15, 2020 22:50

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

CodeBlanch and others added 2 commits August 15, 2020 16:07

Update docs/trace/enriching-activity-processor/README.md

d319e56

Co-authored-by: Reiley Yang <reyang@microsoft.com>

Update docs/trace/enriching-activity-processor/README.md

24ef002

Co-authored-by: Reiley Yang <reyang@microsoft.com>

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

CodeBlanch added 3 commits August 15, 2020 16:15

Doc review.

239e212

Switch span to activity in doc.

caf1dbb

Doc review.

6dbe93e

reyang reviewed Aug 15, 2020

View reviewed changes

docs/trace/enriching-activity-processor/README.md Outdated Show resolved Hide resolved

reyang approved these changes Aug 15, 2020

View reviewed changes

CodeBlanch added 2 commits August 15, 2020 16:31

Code/doc review.

3463097

Removed table.

7c116de

alanwest reviewed Aug 17, 2020

View reviewed changes

alanwest approved these changes Aug 17, 2020

View reviewed changes

Merge branch 'master' into enrichmentscope

505b87d

reyang added the pr:please-merge label Aug 18, 2020

CodeBlanch mentioned this pull request Aug 18, 2020

Baggage + CorrelationContext improvements by Eddy & Mike #1048

Merged

3 tasks

CodeBlanch closed this Aug 19, 2020

CodeBlanch mentioned this pull request Aug 20, 2020

CorrelationContext -> Baggage #1106

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EnrichmentScope & EnrichingActivityProcessor #969

EnrichmentScope & EnrichingActivityProcessor #969

CodeBlanch commented Aug 1, 2020 •

edited

Loading

reyang Aug 1, 2020

CodeBlanch Aug 15, 2020

reyang Aug 15, 2020

alanwest Aug 17, 2020

CodeBlanch Aug 17, 2020

reyang Aug 17, 2020

alanwest Aug 17, 2020

alanwest Aug 17, 2020

CodeBlanch commented Aug 15, 2020

codecov bot commented Aug 15, 2020 •

edited

Loading

reyang left a comment

eddynaka commented Aug 15, 2020

CodeBlanch commented Aug 15, 2020

eddynaka commented Aug 15, 2020

reyang left a comment

reyang commented Aug 15, 2020

alanwest Aug 17, 2020

CodeBlanch Aug 17, 2020

reyang Aug 17, 2020

alanwest Aug 17, 2020

alanwest Aug 17, 2020

CodeBlanch Aug 17, 2020

alanwest left a comment

CodeBlanch commented Aug 19, 2020

CodeBlanch commented Mar 29, 2021

		Note: Order is important. Make sure your `EnrichingActivityProcessor` is
		added before your `Exporter` and any other `ActivityProcessor`s you are using.

EnrichmentScope & EnrichingActivityProcessor #969

EnrichmentScope & EnrichingActivityProcessor #969

Conversation

CodeBlanch commented Aug 1, 2020 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Aug 15, 2020

codecov bot commented Aug 15, 2020 • edited Loading

Codecov Report

reyang left a comment

Choose a reason for hiding this comment

eddynaka commented Aug 15, 2020

CodeBlanch commented Aug 15, 2020

eddynaka commented Aug 15, 2020

reyang left a comment

Choose a reason for hiding this comment

reyang commented Aug 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest left a comment

Choose a reason for hiding this comment

CodeBlanch commented Aug 19, 2020

CodeBlanch commented Mar 29, 2021

CodeBlanch commented Aug 1, 2020 •

edited

Loading

codecov bot commented Aug 15, 2020 •

edited

Loading