Does anything prevent us from doing?

It feels odd to be replicating all this start code.

I think it is possible. The only different case would be when someone calling StartActivity and passing ActivityContext with a default span Id. we used to keep Activity.Parent=null but if we call Activity.Start() it would set Activity.Parent to the Activity.Current. I am not sure though if this will be ok (or should be the right behavior). what you think?

In theory spanId = 0 is an invalid parent context. We should probably take a peak at the spec and chat with the OT folks to see if they have any input on what error handling behavior is desirable. I don't recall us ever defining the behavior very precisely but what I see the code currently do is give the bad id (in string or ActivityContext form) to the sampler as-is. Later in CreateAndStart we potentially ignore the id (if it was in string form) and create a new id based on IdFormat. This runs afoul of our sampler TraceId == Activity.TraceId invariant the same as above.

I don't care too much about the factoring on its own, but I am glad thinking about it sussed out issues like this one : )

Weren't we going to handle #43853 by adding an IdFormat parameter here?

No. After discussing this in the design review, @pakrym felt the other proposal is not needed and that can be handled by this CreateActivity proposal. After creating the activity, they can set the IdFormat on such created Activity before starting it.

Unforetunately that doesn't work. CreateActivity() calls the sampling callback, sampling callback has access to the ID, generating that ID has already fixed the id format. Using SetIdFormat() after CreateActivity() should probably throw an exception if it doesn't already.

        public Activity SetIdFormat(ActivityIdFormat format)
        {
            if (_id != null || _spanId != null)
            {
                NotifyError(new InvalidOperationException(SR.SetFormatOnStartedActivity));
            }
            else
            {
                IdFormat = format;
            }
            return this;
        }

If the Activity not started, _id and _spanId would still be nulls. otherwise Activity.Start() would fail too. no?

@noahfalk I have addressed all comments. The remaining point is this one which related to IdFormat. I don't think we should fix the Ids before starting the Activity. Please let me know if you think otherwise.

The caller is passing default context, there is no parent (Activity.Current), and the listeners didn't try to access the trace Id from the provided ActivityCreationOptions.

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

This case seems very real. Think of a worker app uploading a blob. There is no parent and all spans are exported so TraceId is not accessed.

Yes, agree. But using CreateActivity with the current design will make it work. right? I mean users can use CreateActivity and SetIdFormat if they need to for this scenario.

@pakrym I had offline discussion with @noahfalk and we propose adding extra Boolean parameter to the CreateActivity called forceW3CId. The reason we didn't have the parameter of ActivityIdFormat type because we want to promote W3C in general and not allow users to try the hierarchal explicitly. Please let's know if you have any concern. I'll send it anyway to the fxdc and will CC you there.

        public Activity? CreateActivity(string name, 
                                        ActivityKind kind);

        public Activity? CreateActivity(
                             string name,
                             ActivityKind kind, 
                             ActivityContext parentContext,
                             IEnumerable<KeyValuePair<string, object?>>? tags = null, 
                             IEnumerable<ActivityLink>? links = null,
                             bool forceW3CId = false); // added parameter

        public Activity? CreateActivity(
                             string name, 
                             ActivityKind kind,
                             string parentContext,
                             bool forceW3CId = false,
                             IEnumerable<KeyValuePair<string, object?>>? tags = null, 
                             IEnumerable<ActivityLink>? links = null,
                             bool forceW3CId = false);     // added parameter

LGTM. If we want to promote W3C can we default the parameter to true? 😄

If we want to promote W3C can we default the parameter to true? 😄

I discussed that with @noahfalk and he preferred not to default the value to W3C as he want to have consistent behavior with StartActivity when passing the exact same parameters.

This delegate needs a closure so it will do an allocation that we don't want. However we have a few ways out:

1. Previously the behavior for ActivityCreationOptions<string>.TraceId is that it always returns default. I suggest we preserve that behavior so there is no need to copy the TraceId across the different contexts.
2. Previously generating the TraceId was expensive because our random number generator was slow. Maybe now it is fast enough that there is little advantage in trying to generate it lazily? We'd need a benchmark measurement to know for sure.

Ok, I'll go with the #1 to make ActivityCreationOptions<string>.TraceId return default. I'll remove the delegate from the picture.

There is a missing case in the algorithm where we should choose to use default format id:

• IdFormat passed to ActivitySource.Create is Unknown (step 1 doesn't apply)
• ForceDefaultFormat = false (step 2 doesn't apply)
• Activity.Current = null (step 3 doesn't apply)
• parent ActivityContext/id are default (steps 4 and 5 don't apply)

right. I handled this in the code :-) but didn't specify this step here. I'll add it here.

Nit - the constants were inconsistent in the column:

will fix that.

I got the wrong idea about what these columns meant until I reverse engineered it from the source. This might help clarify the intent.

will do

Lets add a 2nd listener that only implements the Sample callback. This will let us confirm that Sample is always called for W3C formatted activities and never called for Hierarchal ones.

We should have the test case where a non-W3C string id gets passed as the parentId, the child does get W3C format, and the ActivityCreationOptions<ActivityContext>.TraceId property does get viewed inside the Sample callback. That case wasn't previously calling Sample and we need to ensure that now it does work correctly.

You mean when the listener is not providing SampleUsingParentId. right?

Right!

Assuming we keep the previous behavior for SampleWithParentId, activityOptions.TraceId should always return default here.

To clarify, it will return default only in the case the parent Id string cannot parsed to W3C. right?

I was looking at the code more closely and may have misinterpretted the old behavior, trying to review more closely now : )

I did misinterpret and regardless I think the way you changed it will be fine. The behavior of ActivityCreationOptions<string>.TraceId is a bit weird, but this matches it's previous weird behavior and breaking change isn't necessarily any better : )

Nice test : )