-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 a way to the ActivityListeners to add more data to the Activity when it gets created #40339
Comments
CC @lmolkova @noahfalk @cijothomas This proposal is pending the other renames, will try to add it shortly. |
@CodeBlanch to share his ideas. ActivityDataRequest -> ActivitySamplingResult Do consider the following as well: |
@cijothomas you suggestions looks reasonable to me. do you think the enum fields names are still ok? public enum ActivityDataRequest
{
None,
PropagationData,
AllData,
AllDataAndRecorded
} |
OpenTelemetry uses the term Sampled as well, but it has a different meaning than what ours would have if we renamed this way:
I'd recommend we not use the term 'Sampled' because redefining its meaning would add further confusion. |
what about:
and properly I confused you more now :-) |
About adding the Thank you for spotting this, adding
(Especially, in the PropagationData-case, adding tags may be "surprising", but customer properties may make sense from the perspective a the monitoring solution.) This would support samplers that prefer to attach something more than a tag to the activity. An alternative to consider would be to allow a sampler to add a post-creation callback:
it would be then invoked if the sampler set it to something other than
|
About the "sampling" terminology: Monitoring solutions may want to use data reduction techniques other than sampling. They may preferentially treat traces that originated at a particular upstream component, traces that were marked in some special way by some upstream component, or generally "filter" in some non-purely=probabilistic way. Would it make sense to move away from the "sampling" terminology and use "DataReduction" or similar instead, and leave it out where possible? E.g.:
or if we pick the other way:
and
|
Is this a real scenario we are facing today? I am asking because the bar now is very high and would be good to hurry avoid APIs which we didn't spend enough time looking at. The proposed APIs in the issue is a blocking OpenTelemetry scenario, that is why we are considering it now. If you can tell more about your scenario we can look at it if it is really blocking. otherwise, I would suggested to track this for next release. Thanks. |
I forgot to comment on public Action<Activity> InitializationCallback { get; set; } I would avoid doing this one for perf reason. This will make the Activity creation is heavy process when listeners exist. remember we can have multiple listeners. Also, we can add this later if proved is needed. |
I'll keep sampling instead of DataReduction, as OpenTelemetry is using the term sampling. |
Agreed, just wanted to highlight the "felxible" alternative.
The scenario described in open-telemetry/opentelemetry-dotnet#953 is "comply to the spec". That is valid. Yet, thinking beyond that for a moment and considering what did the spec actually want to achieve in spirit: Potentially one could create some custom way to pass state between GetRequestedData and StartActivity and set the tags in StartActivity. But it would be complex and, potentially slow. So the spec aims to allow a sampler (data reducer) to attach the "appropriate" state to the span (activity) at the "right" time, so that it can be carried with it. The scenario here is the same - stay in the spirit with the spec and allow attaching the state at the "right" time. Consider a data reducer that makes a non-trivial decision based on some parameters. The monitoring vendor may expect that this data is serialized along with the span (activity) metadata in some special, structured way. For this, the sampler needs to attach it to the activity. Such structured metadata may include sampling rates and data that influenced the sampling decision and may be relevant to the user while consuming the span information. On a meta-level: if it is too late to add these scenarios, that makes sense. However, if we decide to address the gap identified, and thus change the ActivityCreationOptions struct: if the struct is being changed, would it not be better to do it in the best way we can at this time? If the change does occur - how changing one line is better/safer than two lines? |
@macrogreg custom properties are not part of the specs at all. This is .NET thingy to allow users attach anythings to the Activity.
I want to tell we are changing the APIs now to allow the blocking scenarios. Think about, if we got the APIs right in the first time, we wouldn't change it now. This is the purpose of having preview builds for us to collect feedback on the APIs before we ship it. Adding anything now is risky as we'll not have much chance to fix anything after that. That means we need to address the blocking scenario and not just adding any API we think it is useful. I see the API you are proposing is useful but I am not seeing it urgent or blocking to get it now. Getting feedback from this release will be make us confident on adding such API or it can prove the opposite that it better not adding it. In general in current time, if any API proposal is not urgent or blocking, I would prefer pushing it to the next release. notice I am not pushing back on the API itself more than just the timing. |
True. I was approaching the whole thing from the .NET perspective more than from the OTel perspective. :)
Agree. All I am doing is to provide a data point, and - of course - the decision is up to the .NET team. Honestly, if I owned this, I would consider fixing the terminology (because it is very confusing now and cannot be fix later), but not adding any functional changes. Instead, I would do that changes in a later release. But you likely have a more holistic perspective on this. :) So all I am saying is that he proposed approach feels half-way and either full-way or no-way seems preferable, :) |
Proposed API looks good to me! I'm happy we came around to doing a bit of renaming. I like @tarekgh's I would personally vote for If we rename public enum ActivitySamplingResult
{
None,
Propagate,
Populate,
PopulateAndRecord
} |
We appreciate that. We have the proposal to hear all opinions.
The comment open-telemetry/opentelemetry-dotnet#953 (comment) is saying otherwise. I'll let @lmolkova and @cijothomas comment on that.
Agree. this is what we are trying to do here too.
From the issue open-telemetry/opentelemetry-dotnet#953 this looks blocking to me and we need to provide something to make this scenario work nicely. |
We also have this delegate, what you suggest naming it? public delegate ActivityDataRequest GetRequestedData<T>(ref ActivityCreationOptions<T> options); and |
True. But is the goal to pick the best term, or to use the same words as OTel does? .NET already uses different terms at many places. |
@tarekgh I forgot about the delegate! How about this? public enum ActivitySamplingResult
{
None,
Propagate,
Populate,
PopulateAndRecord
}
public delegate ActivitySamplingResult GetSamplingResult<T>(ref ActivityCreationOptions<T> options);
public GetSamplingResult<string>? ShouldSampleParentId { get; set; }
public GetSamplingResult<ActivityContext>? ShouldSample { get; set; } Something a bit more provocative... public enum ActivitySamplingResult
{
None,
Propagate,
Populate,
PopulateAndRecord
}
public delegate ActivitySamplingResult GetSamplingResult(ref ActivityCreationOptions<ActivityContext> options);
public Func<string, ActivityContext?>? ParseCustomParentId { get; set; }
public GetSamplingResult? ShouldSample { get; set; } What if we changed the ParentId case to be a parse/conversion to ActivityContext which we then pass to the regular ShouldSample? If user doesn't implement that part, or it returns null, we just try it as a regular W3C style. |
ShouldListenTo is bit different - it returns true/false, and the name makes it clear that its expected return type is bool. ShouldSample - gives the feeling that the return type is bool, but the return type is more than that. Hence my preference for |
Naming is hard for me :) Populate and Record - could also be confusing.... |
I think .NET has been closely following OTel terminology. .NET has differences because picking OTel name would have conflicting with something already existing. (eg. Activity.IsAllDataRequested instead of Activity.IsRecording as Recorded already exists and means something else) |
Btw - Our goal is to get the ability to add tags from GetRequestedData callback, to Activity, if it gets created. This enables OTel .NET SDK to implement spec compliant Sampling, and help Liudmila/others who want to leverage this to add weightage to Activity as tag from Sampler. This is a critical issue. Everything else including renaming is not blocker. |
"Best" acknowledges that aligning to OTel isn't the only goal we have, which is accurate. However aligning to OTel is significant among those goals. It indicates a community of users agreed upon an industry standard term and using the same term that others use makes understanding more likely. There would need to be a compelling argument if OTel has a term they use in the same context and we are choosing not to use it. So far the only places we felt that burden had been met is if .NET already defined a different standard term and/or the name can't be changed due to back compat.
Although sampling is commonly implied to be probabilistic the term does have broader usage. Sampling can also be |
I'm flexible, but given all the options I've seen so far this is what I would pick + rationale:
Rationale:
|
Should we add a little more detail somewhere explaining how the callbacks are used? Something like: Before: /// <summary>
/// Set or get the callback used to decide if want to listen to <see cref="Activity"/> objects events which created using <see cref="ActivitySource"/> object.
/// </summary>
public Func<ActivitySource, bool>? ShouldListenTo { get; set; } After: /// <summary>
/// Set or get the callback used to decide if want to listen to <see cref="Activity"/> objects events which created using <see cref="ActivitySource"/> object.
/// </summary>
/// <remarks>
/// <see cref="Activity"> objects created via the selected <see cref="ActivitySource"/> objects will be run through the sampling callbacks (<see cref="Sample"/> & <see cref="SampleUsingParentId"/>). Only <see cref="Activity"/> objects sampled (<see cref="ActivitySamplingResult.PropagationData"/>, <see cref="ActivitySamplingResult.AllData"/>, or <see cref="ActivitySamplingResult.AllDataAndRecorded"/>) will trigger <see cref="ActivityStarted"/> and <see cref="ActivityStopped"/> events.
/// </summary>
public Func<ActivitySource, bool>? ShouldListenTo { get; set; } Just trying to nudge people into the pit of success 😉 |
Thanks all for your discussion and suggestions. I have updated the proposal in the top with what we have concluded. I'll go ahead and start the process to get this approved and implemented. |
I like the idea of using remarks in ShouldListenTo to guide people that Sample will probably be important for them as well : ) |
namespace System.Diagnostics
{
public readonly struct ActivityCreationOptions<T>
{
+ public ActivityTagsCollection SamplingTags { get; }
+ public ActivityTraceId TraceId { get; }
}
public sealed class ActivityListener : IDisposable
{
- public bool AutoGenerateRootContextTraceId { get; set;}
// Renames:
- public GetRequestedData<string>? GetRequestedDataUsingParentId { get; set; }
- public GetRequestedData<ActivityContext>? GetRequestedDataUsingContext { get; set; }
+ public SampleActivity<string>? SampleUsingParentId { get; set; }
+ public SampleActivity<ActivityContext>? Sample { get; set; }
}
- public delegate ActivityDataRequest GetRequestedData<T>(ref ActivityCreationOptions<T> options);
+ public delegate ActivitySamplingResult SampleActivity<T>(ref ActivityCreationOptions<T> options);
} |
Background and Motivation
We have exposed the following API in the
ActivitySource
class to create and start the activity.If there is any listener to the Activity events, the parameters passed to
StartActivity
will be sent to the listener to decide if want to create the Activity object and with what initial contents.OpenTelemetry samplers create a listener to sample in or out the Activities. We have got a new OpenTelemetry sampler scenario which is, besides deciding to sample in or out, it also needs to add some more tags to the activity when it gets created. That is means we'll need to have a way in our APIs to allow the listeners to pass back some more data we include in the Activity object we'll create later.
Currently, the callback to the listener has the signature:
Where
ActivityCreationOptions
is a struct that wraps the parameters passed toStartActivity
. CurrentlyActivityCreationOptions
is a read-only struct. The proposal here is to make it read/write struct and adding some extra properties the listeners/sampler can use to stick the extra data (e.g. the extra tags).When having
ActivityCreationOptions
as read/write, this will allow us in the future support communicating more data.Also, we previously exposed
ActivityListener.AutoGenerateRootContextTraceId
property to allow the listener to communicate if need automatically generating a trace id when we have a root parent. It could be not obvious the purpose of this property when looking at the interfaces. So, we decided to enhance this after havingActivityCreationOptions
became read/write. The idea is to get rid of the propertyAutoGenerateRootContextTraceId
in the listener and then have a TraceId property on theActivityCreationOptions
. The property will be a getter only and internally we'll encapsulate the logic of generating a new trace Id when needed.Last, we are proposing to enhance some of the names used in our APIs.
Proposed API
We are removing the property
We are renaming the following methods/types:
ActivityListener.GetRequestedDataUsingParentId
toSampleUsingParentId
ActivityListener.GetRequestedDataUsingContext
toSample
The enum
ActivityDataRequest
toActivitySamplingResult
The delegate
GetRequestedData
toSampleActivity
Before renaming, we had the following:
After renaming we'll have:
The text was updated successfully, but these errors were encountered: