-
Notifications
You must be signed in to change notification settings - Fork 765
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
Changes from 18 commits
1605ace
37f6df5
a34a80c
7483e90
4d7fd7d
ec65bc0
1964da6
685c298
a26e2ce
de2898e
1ab0969
3a1f65b
387406a
6454533
a07482c
0fecef0
2a02388
6f5f2f1
90ca374
a5196e7
9b815e7
0503bbb
172dd1b
f48bc05
f9a3f8b
2f6a3ff
80c56b8
34c6667
1a8a922
8c95541
302267f
bb25ad0
374b4b0
e913a56
f8c967b
c3f05f8
4fb391a
eb85991
1b3b49f
0a283bf
3c2e9fd
07b8b46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
|
||
namespace OpenTelemetry.Context | ||
|
@@ -25,46 +26,73 @@ 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; | ||
|
||
internal CorrelationContext(in Activity activity) | ||
{ | ||
this.activity = activity; | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="CorrelationContext"/> struct. | ||
/// Gets the current <see cref="CorrelationContext"/>. | ||
/// </summary> | ||
/// <param name="entries">Entries for correlation context.</param> | ||
internal CorrelationContext(List<CorrelationContextEntry> entries) | ||
public static CorrelationContext Current | ||
{ | ||
this.entries = entries; | ||
get | ||
{ | ||
Activity activity = Activity.Current; | ||
return activity == null | ||
? Empty | ||
: new CorrelationContext(activity); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Gets empty object of <see cref="CorrelationContext"/> struct. | ||
/// Gets the correlation values. | ||
/// </summary> | ||
public static CorrelationContext Empty { get; } = new CorrelationContext(EmptyList); | ||
public IEnumerable<KeyValuePair<string, string>> Correlations => this.activity?.Baggage ?? EmptyBaggage; | ||
|
||
/// <summary> | ||
/// Gets all the <see cref="CorrelationContextEntry"/> in this <see cref="CorrelationContext"/>. | ||
/// Retrieves a correlation item. | ||
/// </summary> | ||
public IEnumerable<CorrelationContextEntry> Entries => this.entries; | ||
/// <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> | ||
/// Gets the <see cref="CorrelationContextEntry"/> with the specified name. | ||
/// Adds a correlation item. | ||
/// </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="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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
/// <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; | ||
} | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?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.
I think so. The enumeration is returning the KVPs. They are structs, so you essentially get a copy?