Use current activity if there is one

[russcam]

This commit fixes a bug whereby Activity.Current should be used
to set the TraceId if

1. it's not null
2. it's using W3C Id format
3. activity are not being ignored

[russcam]

This is needed for #1225

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #1228 updated

• Start Time: 2021-03-23T05:27:12.968+0000

• Duration: 53 min 28 sec

• Commit: eb4f85b

Test stats 🧪

Test	Results
Failed	0
Passed	18759
Skipped	80
Total	18839

Trends 🧪

[gregkalapos]

Overall it looks good to me. I left 1 minor thing to add a comment and another one (that's the more important) about setting the recorded flag on the already existing Activity.

Jenkins also showed failing gRPC tests, I looked into that - I found the problem within WebRequestTransactionCreator.cs, I opened a PR against your branch: russcam#1 - I think that'll fix the gRPC problem.

[russcam]

#1235 has made me think about whether we should create a new activity or not, based on there being an existing activity that meets the criteria, vs. always creating a new activity, but reusing the traceid and tracestate of the existing activity, if it meets criteria. I'm thinking about this from a user's usability perspective - I don't have a good handle on what a user might want to do with the current Activity inside of an APM transaction, and whether they might need to know if a new activity has been started or not. What are your thoughts, @gregkalapos?

[gregkalapos]

#1235 has made me think about whether we should create a new activity or not, based on there being an existing activity that meets the criteria, vs. always creating a new activity, but reusing the traceid and tracestate of the existing activity, if it meets criteria. I'm thinking about this from a user's usability perspective - I don't have a good handle on what a user might want to do with the current Activity inside of an APM transaction, and whether they might need to know if a new activity has been started or not. What are your thoughts, @gregkalapos?

That's a good point. I feel the most important thing is that Activity.Current.TraceId should be in-sync with the traceid we use, that was the main motivation behind messing with Activity during transaction creation. If we'd always create an activity, that'll still be true an it'd be consistent - so the agent would always fires an activity on transaction creation.

always creating a new activity, but reusing the traceid and tracestate of the existing activity

If there is already an activity A and we create a new activity B, then traceid and tracestate will be the same on both I think, because traceid and tracestate flow to child activities.

A related thing that comes to my mind:
The other thing that happens is that the Id of our transaction is the same as the spanId of the Activity the agent fires. We were already asked why our transaction id is different from the Activity.SpanId when there is an already existing Activity. Here I think if we reuse Activity.SpanId we should only do that for the Activity that we fire in the agent. Reason is that the transaction is different from any other Activity, specifically its duration may be completely different.

So, in sum: I think always creating an activity won't change a lot here, and it's be more consistent.

Regardless of this, I'd keep the code from #1235 where we walk the Activity chain to find the right one for gRPC info.

[gregkalapos]

LGTM 👍

[russcam]

So, in sum: I think always creating an activity won't change a lot here, and it's be more consistent.

I've just committed eb4f85b to always create and start a new Activity, if we're not ignoring them.

This commit also tries to set the ParentId of the created Activity to the DistributedTracingData, if passed; this will overwrite the ParentSpanId and TraceId of the created activity, that may have been set from Activity.Current if it was not null when the created activity was started. This seems like the right thing to do, in order to keep explicitly passed tracing data in sync for the Transaction and Activity.

[russcam]

Would you mind taking this another look, @gregkalapos? I think it's good to go, but interested in your thoughts on the changes in eb4f85b