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

Baggage + CorrelationContext improvements by Eddy & Mike #1048

Merged
merged 42 commits into from
Aug 18, 2020
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
1605ace
Support W3C Baggage spec.
CodeBlanch Aug 9, 2020
37f6df5
Moved baggage propagation to its own ITextFormat. Removed IsInjected.
CodeBlanch Aug 9, 2020
a34a80c
updating some tests
eddynaka Aug 9, 2020
7483e90
creating nw files
eddynaka Aug 9, 2020
4d7fd7d
updating files
eddynaka Aug 9, 2020
ec65bc0
buildable in release
eddynaka Aug 9, 2020
1964da6
adding baggage tests
eddynaka Aug 10, 2020
685c298
updating tests
eddynaka Aug 10, 2020
a26e2ce
updating default textformat for http instrumentation
eddynaka Aug 10, 2020
de2898e
Removed a few null checks.
CodeBlanch Aug 11, 2020
1ab0969
Removed DistributedContext. Drive CorrelationContext off of Activity.…
CodeBlanch Aug 11, 2020
3a1f65b
Merge branch 'master' into baggage
eddynaka Aug 11, 2020
387406a
Merge branch 'baggage' of https://github.com/CodeBlanch/opentelemetry…
eddynaka Aug 11, 2020
6454533
updating issues after merge
eddynaka Aug 11, 2020
a07482c
updating based on sanity check
eddynaka Aug 11, 2020
0fecef0
updating baggage test
eddynaka Aug 11, 2020
2a02388
updating tests
eddynaka Aug 11, 2020
6f5f2f1
reiley's comments
eddynaka Aug 11, 2020
90ca374
move to using
eddynaka Aug 11, 2020
a5196e7
Merge branch 'master' into baggage
CodeBlanch Aug 12, 2020
9b815e7
Updates for http-in and http-out. Updated CHANGELOGs.
CodeBlanch Aug 12, 2020
0503bbb
Merge branch 'master' into baggage
eddynaka Aug 12, 2020
172dd1b
Adding tests.
CodeBlanch Aug 13, 2020
f48bc05
Merge branch 'master' into baggage
eddynaka Aug 13, 2020
f9a3f8b
updating correlation context
eddynaka Aug 13, 2020
2f6a3ff
Added test for TraceContextFormat + BaggageFormat used together.
CodeBlanch Aug 14, 2020
80c56b8
Merging from master.
CodeBlanch Aug 14, 2020
34c6667
Fixed broken tests.
CodeBlanch Aug 14, 2020
1a8a922
Code review.
CodeBlanch Aug 14, 2020
8c95541
Test fixup.
CodeBlanch Aug 14, 2020
302267f
updating order
eddynaka Aug 14, 2020
bb25ad0
updating tests
eddynaka Aug 14, 2020
374b4b0
updating tests, adding dispose, clearing objects
eddynaka Aug 14, 2020
e913a56
Merge branch 'master' into baggage
CodeBlanch Aug 15, 2020
f8c967b
Merge branch 'master' into baggage
cijothomas Aug 18, 2020
c3f05f8
Merge branch 'master' into baggage
cijothomas Aug 18, 2020
4fb391a
updating changelog
eddynaka Aug 18, 2020
eb85991
Use "Baggage" instead of "baggage" as the header name.
CodeBlanch Aug 18, 2020
1b3b49f
Added some basic support for the Baggage limits specified in the spec.
CodeBlanch Aug 18, 2020
0a283bf
Fixed and improved ITextFormat log messages.
CodeBlanch Aug 18, 2020
3c2e9fd
Rename TextFormatContext -> PropagationContext.
CodeBlanch Aug 18, 2020
07b8b46
Updated ITextFormat implementations so they don't double-extract.
CodeBlanch Aug 18, 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
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public void ReceiveMessage(BasicDeliverEventArgs ea)
// https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/messaging.md#span-name
var activityName = $"{ea.RoutingKey} receive";

using (var activity = ActivitySource.StartActivity(activityName, ActivityKind.Consumer, parentContext))
using (var activity = ActivitySource.StartActivity(activityName, ActivityKind.Consumer, parentContext.ActivityContext))
{
try
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public string SendMessage()
if (activity != null)
{
// Inject the ActivityContext into the message headers to propagate trace context to the receiving service.
TextFormat.Inject(activity.Context, props, this.InjectTraceContextIntoBasicProperties);
TextFormat.Inject(new TextFormatContext(activity.Context, activity.Baggage), props, this.InjectTraceContextIntoBasicProperties);

// The OpenTelemetry messaging specification defines a number of attributes. These attributes are added here.
RabbitMqHelper.AddMessagingTags(activity);
Expand Down
7 changes: 7 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

## Unreleased

* Added `BaggageFormat` an `ITextFormat` implementation for managing Baggage
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
propagation via the [W3C
Baggage](https://github.com/w3c/baggage/blob/master/baggage/HTTP_HEADER_FORMAT.md)
header
([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048))
* Removed `DistributedContext` as it is no longer part of the spec
([#1048](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1048)))
* Renaming from `ot` to `otel`
([#1046](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1046))
* Added `RuntimeContext` API
Expand Down
86 changes: 66 additions & 20 deletions src/OpenTelemetry.Api/Context/CorrelationContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;

namespace OpenTelemetry.Context
Expand All @@ -25,27 +26,33 @@ namespace OpenTelemetry.Context
/// </summary>
public readonly struct CorrelationContext : IEquatable<CorrelationContext>
{
private static readonly List<CorrelationContextEntry> EmptyList = new List<CorrelationContextEntry>();
private readonly List<CorrelationContextEntry> entries;
internal static readonly CorrelationContext Empty = new CorrelationContext(null);
internal static readonly IEnumerable<KeyValuePair<string, string>> EmptyBaggage = new KeyValuePair<string, string>[0];
private readonly Activity activity;

/// <summary>
/// Initializes a new instance of the <see cref="CorrelationContext"/> struct.
/// </summary>
/// <param name="entries">Entries for correlation context.</param>
internal CorrelationContext(List<CorrelationContextEntry> entries)
internal CorrelationContext(in Activity activity)
{
this.entries = entries;
this.activity = activity;
}

/// <summary>
/// Gets empty object of <see cref="CorrelationContext"/> struct.
/// Gets the current <see cref="CorrelationContext"/>.
/// </summary>
public static CorrelationContext Empty { get; } = new CorrelationContext(EmptyList);
public static CorrelationContext Current
{
get
{
Activity activity = Activity.Current;
return activity == null
? Empty
: new CorrelationContext(activity);
}
}

/// <summary>
/// Gets all the <see cref="CorrelationContextEntry"/> in this <see cref="CorrelationContext"/>.
/// Gets the correlation values.
/// </summary>
public IEnumerable<CorrelationContextEntry> Entries => this.entries;
public IEnumerable<KeyValuePair<string, string>> Correlations => this.activity?.Baggage ?? EmptyBaggage;
Copy link
Member

Choose a reason for hiding this comment

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

Couple questions:

Copy link
Member Author

Choose a reason for hiding this comment

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

Would there be memory allocation concern?

Check out the underlying Activity.Baggage enumeration:
https://github.com/dotnet/runtime/blob/ead2cd50799ecc523e23f50e50b54d8a30d0aa4f/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L290

Right below that method is the get.

The memory won't be too bad. An allocation for the state thing generated by the compiler due to the yield. The internal storage is a LinkedList, not great for searching. O(N)?

What would happen if a consumer gets the IEnumerable and tries to iterate through it, while the underlying baggage got modified?

I think the way the LinkedList works under the hood, it would actually be OK. User can only add to the back of the list. So if you were enumerating you would also consume that new one when you got to the tail.

Does this enforce immutability?

I think so. The enumeration is returning the KVPs. They are structs, so you essentially get a copy?


/// <summary>
/// Compare two entries of <see cref="CorrelationContext"/> for equality.
Expand All @@ -62,23 +69,62 @@ internal CorrelationContext(List<CorrelationContextEntry> entries)
public static bool operator !=(CorrelationContext left, CorrelationContext right) => !(left == right);

/// <summary>
/// Gets the <see cref="CorrelationContextEntry"/> with the specified name.
/// Retrieves a correlation item.
/// </summary>
/// <param name="key">Correlation item key.</param>
/// <returns>Retrieved correlation value or <see langword="null"/> if no match was found.</returns>
public string GetCorrelation(string key)
=> this.activity?.GetBaggageItem(key);

/// <summary>
/// Adds a correlation item.
/// </summary>
/// <param name="key">Correlation item key.</param>
/// <param name="value">Correlation item value.</param>
/// <returns>The <see cref="CorrelationContext"/> instance for chaining.</returns>
public CorrelationContext AddCorrelation(string key, string value)
Copy link
Member

Choose a reason for hiding this comment

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

This API will not match the spec, right? As spec wants "Set" behavior, but we have "add" behavior, and also we won't have "Remove" feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. The current version is built on top of Activity.Baggage, which is very limiting. Let's discuss this. The other option is we could make CorrelationContext independent of Baggage which would allow us to fully implement the spec. If we do that, let's remove "AllChildren" from #969. CorrelationContext could be used for "AllChildren" behavior and then EnrichmentScope is just responsible for "FirstChild" behavior?

{
this.activity?.AddBaggage(key, value);

return this;
}

/// <summary>
/// Adds correlation items.
/// </summary>
/// <param name="key">Name of the <see cref="CorrelationContextEntry"/> to get.</param>
/// <returns>The <see cref="string"/> with the specified name. If not found - null.</returns>
public string GetEntryValue(string key) => this.entries.LastOrDefault(x => x.Key == key).Value;
/// <param name="correlations">Correlation items.</param>
/// <returns>The <see cref="CorrelationContext"/> instance for chaining.</returns>
public CorrelationContext AddCorrelation(IEnumerable<KeyValuePair<string, string>> correlations)
{
if (correlations != null)
{
foreach (KeyValuePair<string, string> correlation in correlations)
{
this.activity?.AddBaggage(correlation.Key, correlation.Value);
}
}

return this;
}

/// <inheritdoc/>
public bool Equals(CorrelationContext other)
{
if (this.entries.Count != other.entries.Count)
var thisCorrelations = this.Correlations;
var otherCorrelations = other.Correlations;

if (thisCorrelations.Count() != otherCorrelations.Count())
{
return false;
}

foreach (CorrelationContextEntry entry in this.entries)
var thisEnumerator = thisCorrelations.GetEnumerator();
var otherEnumerator = otherCorrelations.GetEnumerator();

while (thisEnumerator.MoveNext() && otherEnumerator.MoveNext())
{
if (other.GetEntryValue(entry.Key) != entry.Value)
if (thisEnumerator.Current.Key != otherEnumerator.Current.Key
|| thisEnumerator.Current.Value != otherEnumerator.Current.Value)
{
return false;
}
Expand All @@ -96,7 +142,7 @@ public override bool Equals(object obj)
/// <inheritdoc/>
public override int GetHashCode()
{
return this.entries.GetHashCode();
return this.Correlations.GetHashCode();
}
}
}
Loading