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

EnrichmentScope & EnrichingActivityProcessor #969

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
73cee9b
Added EnrichmentScope.
CodeBlanch Aug 1, 2020
66e1724
Tweak EnrichmentScope so it doesn't flow to children where it shouldn't.
CodeBlanch Aug 3, 2020
d52bec5
Merging from master.
CodeBlanch Aug 15, 2020
35a1a9c
Incorporating feedback.
CodeBlanch Aug 15, 2020
b4d6532
Fixed potential null ref exception.
CodeBlanch Aug 15, 2020
1b16067
OnEnd isn't called unless activity.IsAllDataRequested is already true.
CodeBlanch Aug 15, 2020
320c946
Code review.
CodeBlanch Aug 15, 2020
f33057c
Parent null check.
CodeBlanch Aug 15, 2020
651b4c7
Bug fixes and the first round of tests.
CodeBlanch Aug 15, 2020
04a86dd
Updated changelog.
CodeBlanch Aug 15, 2020
e682a6a
Merge branch 'master' into enrichmentscope
CodeBlanch Aug 15, 2020
6b88c9f
Test artifact cleanup.
CodeBlanch Aug 15, 2020
7eeaf58
Out of order disposal tests.
CodeBlanch Aug 15, 2020
7f76856
Added log message for exception thrown enriching activity.
CodeBlanch Aug 15, 2020
793a98c
Fixing spots broken by namespace change.
CodeBlanch Aug 15, 2020
96f9d30
Removed example code.
CodeBlanch Aug 15, 2020
60448bd
Renamed "NextActivity" -> "FirstChild". Added an example + README.
CodeBlanch Aug 15, 2020
102c6b1
Markdown fixup.
CodeBlanch Aug 15, 2020
d319e56
Update docs/trace/enriching-activity-processor/README.md
CodeBlanch Aug 15, 2020
24ef002
Update docs/trace/enriching-activity-processor/README.md
CodeBlanch Aug 15, 2020
239e212
Doc review.
CodeBlanch Aug 15, 2020
caf1dbb
Switch span to activity in doc.
CodeBlanch Aug 15, 2020
6dbe93e
Doc review.
CodeBlanch Aug 15, 2020
3463097
Code/doc review.
CodeBlanch Aug 15, 2020
7c116de
Removed table.
CodeBlanch Aug 15, 2020
505b87d
Merge branch 'master' into enrichmentscope
cijothomas Aug 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion examples/AspNetCore/Controllers/WeatherForecastController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using System.Net.Http;
using Examples.AspNetCore.Models;
using Microsoft.AspNetCore.Mvc;
using OpenTelemetry.Trace;

namespace Examples.AspNetCore.Controllers
{
Expand All @@ -35,8 +36,24 @@ public class WeatherForecastController : ControllerBase
private static HttpClient httpClient = new HttpClient();

[HttpGet]
public IEnumerable<WeatherForecast> Get()
public IEnumerable<WeatherForecast> Get(int? zipPostalCode = null)
{
using var scope = EnrichmentScope.Begin(
target: EnrichmentScopeTarget.NextActivity,
enrichmentAction: a =>
{
if (zipPostalCode.HasValue)
{
a.AddTag("user.zip_postal", zipPostalCode.Value);
}

HttpRequestMessage request = (HttpRequestMessage)a.GetCustomProperty("HttpWebRequest.Request");
if (request != null)
{
a.AddTag("http.content_type", request.Content.Headers.ContentType.ToString());
}
});

// Making an http call here to serve as an example of
// how dependency calls will be captured and treated
// automatically as child of incoming request.
Expand Down
50 changes: 50 additions & 0 deletions src/OpenTelemetry/Trace/EnrichingActivityProcessor.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// <copyright file="EnrichingActivityProcessor.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;

namespace OpenTelemetry.Trace
{
/// <summary>
/// An <see cref="ActivityProcessor"/> implementation that enriches <see cref="Activity"/> objects using <see cref="EnrichmentScope"/>s.
/// </summary>
public class EnrichingActivityProcessor : ActivityProcessor
{
/// <inheritdoc/>
public override void OnEnd(Activity activity)
{
EnrichmentScope scope = EnrichmentScope.Current;
while (scope != null)
{
try
{
scope.EnrichmentAction?.Invoke(activity);
}
catch
{
// todo: Log
}

if (scope.EnrichmentTarget == EnrichmentScopeTarget.NextActivity)
{
scope.Dispose();
}

scope = scope.Parent;
}
}
}
}
100 changes: 100 additions & 0 deletions src/OpenTelemetry/Trace/EnrichmentScope.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// <copyright file="EnrichmentScope.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>
using System;
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
using System.Diagnostics;
using OpenTelemetry.Context;

namespace OpenTelemetry.Trace
{
/// <summary>
/// A class for enriching the data of <see cref="Activity"/> objects.
/// </summary>
public class EnrichmentScope : IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yes! agreed.

{
private static readonly RuntimeContextSlot<EnrichmentScope> RuntimeContextSlot = RuntimeContext.RegisterSlot<EnrichmentScope>("otel.enrichment_scope");

private bool disposed;

/// <summary>
/// Initializes a new instance of the <see cref="EnrichmentScope"/> class.
/// </summary>
/// <param name="enrichmentAction">Callback action for performing <see cref="Activity"/> enrichment.</param>
/// <param name="target">The target for the enrichment scope.</param>
private EnrichmentScope(Action<Activity> enrichmentAction, EnrichmentScopeTarget target)
{
this.EnrichmentAction = enrichmentAction ?? throw new ArgumentNullException(nameof(enrichmentAction));
this.EnrichmentTarget = target;

this.Parent = Current;
this.Parent.Child = this;

RuntimeContextSlot.Set(this);
}

/// <summary>
/// Gets the current <see cref="EnrichmentScope"/>.
/// </summary>
public static EnrichmentScope Current => RuntimeContextSlot.Get();

/// <summary>
/// Gets the parent <see cref="EnrichmentScope"/>.
/// </summary>
public EnrichmentScope Parent { get; internal set; }

/// <summary>
/// Gets the child <see cref="EnrichmentScope"/>.
/// </summary>
public EnrichmentScope Child { get; internal set; }

/// <summary>
/// Gets the enrichment action.
/// </summary>
public Action<Activity> EnrichmentAction { get; private set; }

/// <summary>
/// Gets the enrichment target.
/// </summary>
public EnrichmentScopeTarget EnrichmentTarget { get; private set; }

/// <summary>
/// Registers an action that will be called to enrich the next <see cref="Activity"/> processed under the current scope if it has been sampled.
/// </summary>
/// <param name="enrichmentAction">Action to be called.</param>
/// <param name="target">The target for the enrichment scope.</param>
/// <returns><see cref="IDisposable"/> to cancel the enrichment scope.</returns>
public static IDisposable Begin(Action<Activity> enrichmentAction, EnrichmentScopeTarget target = EnrichmentScopeTarget.NextActivity)
=> new EnrichmentScope(enrichmentAction, target);

/// <inheritdoc/>
public void Dispose()
{
if (!this.disposed)
{
this.EnrichmentAction = null;

this.Parent.Child = this.Child;
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
if (this.Child != null)
{
this.Child.Parent = this.Parent;
}

RuntimeContextSlot.Set(this.Parent);

this.disposed = true;
}
}
}
}
36 changes: 36 additions & 0 deletions src/OpenTelemetry/Trace/EnrichmentScopeTarget.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// <copyright file="EnrichmentScopeTarget.cs" company="OpenTelemetry Authors">
// Copyright The OpenTelemetry Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// </copyright>

using System.Diagnostics;

namespace OpenTelemetry.Trace
{
/// <summary>
/// Describes the target of an <see cref="EnrichmentScope"/>.
/// </summary>
public enum EnrichmentScopeTarget
{
/// <summary>
/// The next <see cref="Activity"/> created will be enriched and then the scope will automatically be closed.
/// </summary>
NextActivity,

/// <summary>
/// All children created under the scope will be enriched until the scope is closed.
/// </summary>
AllChildren,
}
}