RUMM-612 Add active span concept to Tracer #187

nachoBonafonte · 2020-07-17T08:22:01Z

What and why?

When a Span is created without parent it assigns the active span of the current execution context if exists, so spans can now inherit from a span even when they initially don't know about it, or keep a reference to it

How?

The implemantation uses os.activity framework to create activities when the spans are started, and keep a reference to this id in SpanContext.
ActiveSpanUtils keep a map with all the relation activity_id and Spans, where spans are added on creation and removed on finish.
When we want to know the active Span we get the current activity and locate the Span associated with it
Also changed some tests that were failing because spans were not being finished properly and next test was inheriting from previous spans creates

Review checklist

Feature or bugfix MUST have appropriate tests (unit, integration)
Make sure each commit and the PR mention the Issue number or JIRA reference

When a Span is created without parent it assigns the active span of the current execution context if exists, so spans can now inherit from a span even when they initially don't know about it, or keep a reference. The implemantation uses os.activity framework to create activities when the spans are started, and keep a reference to this id in SpanContext. ActiveSpanUtils keep a map with all the relation activity_id and Spans, where spans are added on creation and removed on finish. When we want to know the active Span we get the current activity and locate the Span associated with it

buranmert

it's not that i request changes, i just didn't understand why we need os_activity code
we could keep track of all the spans created with a singleton object instead of relying on os_activity, am i missing something?

Sources/Datadog/Tracing/DDSpanContext.swift

Sources/Datadog/Tracer.swift

Sources/Datadog/Tracing/DDSpanContext.swift

ncreated

I like this feature 💪👌, it's so powerful 😱!

Beside CR comments, I have some more notes on it:

Flakiness

I noticed a flakiness in TracerTests when running all tests locally. Let's get rid of it as well 👌.

`os.activity` APIs

I can see some APIs which seem unsafe. Are we sure this won't put our users in trouble with Apple validation? This can be also checked by dogfooding it in our mobile app.

Backward compatibility

As we are using few unsafe APIs in there - I think we should dogfood it well before merging to master. Because we're close to Tracing GA, this might mean that this feature will be shipped in 1.4.0. And here I'm not sure, as this change doesn't look backward-compatible: if users start using 1.3.0, the 1.4.0 will drastically change their traces on app.datadoghq.com, no? WDYT on this point @buranmert @nachoBonafonte ?

All platform tests

The CI is red, let's fix it and run all-platform-tests after.

Sources/Datadog/Tracer.swift

Sources/Datadog/Tracing/DDSpanContext.swift

Sources/Datadog/Tracing/Utils/ActiveSpanUtils.swift

Sources/Datadog/Tracer.swift

Sources/Datadog/Tracing/Utils/ActiveSpanUtils.swift

Tests/DatadogTests/Datadog/Tracing/Utils/ActiveSpanUtilsTest.swift

nachoBonafonte · 2020-07-17T11:16:10Z

About the backward compatibility, yes, it should probably be evaluated and check throroughly before adding to master, probably 1.4.0 is a good timeframe. The change to the spans will be related to the span parents, it could probably change results but probably would mean that will improve the information for free (if used the traces properly and finished as they should)

Sources/Datadog/Tracing/Utils/ActiveSpanUtils.swift

ncreated · 2020-07-17T14:00:55Z

About the backward compatibility, yes, it should probably be evaluated and check throroughly before adding to master, probably 1.4.0 is a good timeframe. The change to the spans will be related to the span parents, it could probably change results but probably would mean that will improve the information for free (if used the traces properly and finished as they should)

That's true. It will only impact the code that doesn't reference parent span explicitly 👍.

Sources/_Datadog_Private/ObjcOSActivityUtils.m

Sources/Datadog/Tracing/DDSpanContext.swift

resolved

nachoBonafonte · 2020-08-04T12:52:53Z

@test-all-platforms

nachoBonafonte · 2020-08-04T13:04:31Z

@test-all-platforms

ncreated · 2020-08-04T13:07:01Z

@test-all-platforms

ncreated · 2020-08-04T13:09:59Z

@test-all-platforms nachoBonafonte/RUMM-612-Add-active-span-concept-to-Tracer

Sources/Datadog/Tracing/DDSpanContext.swift

Sources/Datadog/Tracing/Utils/ActiveSpansPool.swift

nachoBonafonte · 2020-08-04T14:12:35Z

@test-all-platforms nachoBonafonte/RUMM-612-Add-active-span-concept-to-Tracer

buranmert · 2020-08-04T16:53:45Z

we may try to fix that thread sanitizer issue when we have time one day 🎗️

ncreated

Please also squash the commits so their names follow the guideline:

Make sure each commit and the PR mention the Issue number or JIRA reference

Sources/Datadog/OpenTracing/OTTracer.swift

Sources/Datadog/Tracing/Utils/ActiveSpansPool.swift

ncreated · 2020-08-05T08:45:33Z

We also need to fix one more thing in SendTracesFixtureViewController (Example app target). Before, it was sending 3 separate traces for URLSession requests:

Now, they get linked as a single trace:

This is because the fixture starts next requests without making sure the previous one did complete. Let's keep them separate (do one by one), so we keep the clean preview of different span configuration in APM Traces Explorer.

buranmert · 2020-08-05T08:51:35Z

@ncreated SendTracesFixtureViewController makes the requests sequentially but we call completion first and then span.finish() in auto-instrumented requests. with activeSpan concept it makes more sense to span.finish() first and then call completion 👍

the required change is in URLSessionSwizzler.swift line 73 and 117
can we make the change in this PR @nachoBonafonte or shall i raise another PR for that?

nachoBonafonte · 2020-08-05T09:06:37Z

Yes, I can make the change as part of the PR

nachoBonafonte · 2020-08-05T09:35:18Z

Added latest changes

…re, they were created with a existing activeSpan

Co-authored-by: Maciek Grzybowski <[email protected]>

- make activeSpanPool a standard class and a member of the tracer - Make activeSPan part of the OTTracer standard

…he Tracer + real request captured by intercepting the URLSession's protocol

…cky solutions in swift

…anitizer hung

…nd DDSpanContext. Convert activityId and activityState in private members Clean currentActivity logic in ObjcOSActivityUtils, static var not needed

… if ThreadSanitizer doesn't fail

… in Swift code

…d to see if fixes Threadsanitizer

…chronized to see if fixes Threadsanitizer" This reverts commit 30d57fb.

…thread sanitizer hung" This reverts commit db9de27.

This reverts commit 54044e4.

…iginal completion method Address some more PR comments

…orrect after previous changes

After fixing URLSessionSwizzler finishing spans, this spans are now root spans

ncreated

🚀

nachoBonafonte requested a review from a team as a code owner July 17, 2020 08:22

nachoBonafonte self-assigned this Jul 17, 2020

buranmert suggested changes Jul 17, 2020

View reviewed changes

Sources/Datadog/Tracing/DDSpanContext.swift Outdated Show resolved Hide resolved

Sources/Datadog/Tracer.swift Outdated Show resolved Hide resolved

Sources/Datadog/Tracing/DDSpanContext.swift Outdated Show resolved Hide resolved

ncreated requested changes Jul 17, 2020

View reviewed changes

buranmert previously requested changes Jul 17, 2020

View reviewed changes

Sources/Datadog/Tracing/Utils/ActiveSpanUtils.swift Outdated Show resolved Hide resolved

buranmert reviewed Aug 4, 2020

View reviewed changes

Sources/_Datadog_Private/ObjcOSActivityUtils.m Outdated Show resolved Hide resolved

buranmert reviewed Aug 4, 2020

View reviewed changes

Sources/Datadog/Tracing/DDSpanContext.swift Outdated Show resolved Hide resolved

buranmert reviewed Aug 4, 2020

View reviewed changes

Sources/Datadog/Tracing/DDSpanContext.swift Outdated Show resolved Hide resolved

Sources/Datadog/Tracing/Utils/ActiveSpansPool.swift Outdated Show resolved Hide resolved

Sources/Datadog/Tracing/Utils/ActiveSpansPool.swift Outdated Show resolved Hide resolved

nachoBonafonte requested a review from ncreated August 4, 2020 14:12

buranmert approved these changes Aug 4, 2020

View reviewed changes

ncreated requested changes Aug 5, 2020

View reviewed changes

Sources/Datadog/OpenTracing/OTTracer.swift Outdated Show resolved Hide resolved

Sources/Datadog/Tracing/Utils/ActiveSpansPool.swift Outdated Show resolved Hide resolved

nachoBonafonte requested a review from ncreated August 5, 2020 09:38

Nacho Bonafonte and others added 4 commits August 5, 2020 14:41

RUMM-612 Fix a test, request and error spans are not root spans anymo…

e958f3c

…re, they were created with a existing activeSpan

RUMM-612 Apply suggestions from code review

2b595c0

Co-authored-by: Maciek Grzybowski <[email protected]>

RUMM-612 Added some of the changed asket in the review:

7c546d5

- make activeSpanPool a standard class and a member of the tracer - Make activeSPan part of the OTTracer standard

RUMM-612 Added a requested test to TracerTests using public APIs of t…

00077e9

…he Tracer + real request captured by intercepting the URLSession's protocol

Nacho Bonafonte added 15 commits August 5, 2020 14:42

RUMM-612 Move all Obj-C bits to _Datadog_Private module, and avoid ha…

16b1007

…cky solutions in swift

RUMM-612 Missed a license header in a file

a15d520

RUMM-612 Run Thread sanitizer with symbolication

a30ed8b

RUMM-612 Disable code coverage, it can make symbolication of thread s…

12d0366

…anitizer hung

RUMM-612 Move all activity logic to DDSpan, removing it from Tracer a…

50d0b93

…nd DDSpanContext. Convert activityId and activityState in private members Clean currentActivity logic in ObjcOSActivityUtils, static var not needed

RUMM-612 Try removing ObjcOSActivityUtils intermediate class to check…

fecec6a

… if ThreadSanitizer doesn't fail

RUMM-612 Remove ObjcOSActivityUtils, and recreate OS_ACTIVITY_CURRENT…

511e994

… in Swift code

RUMM-612 Add changes from PR review

61894b4

RUMM-612 Trying again with ObjcOSActivityUtils, but using synchronize…

a2c3d18

…d to see if fixes Threadsanitizer

RUMM-612 Revert "Trying again with ObjcOSActivityUtils, but using syn…

82d9b58

…chronized to see if fixes Threadsanitizer" This reverts commit 30d57fb.

RUMM-612 Revert "Disable code coverage, it can make symbolication of …

9efdcd8

…thread sanitizer hung" This reverts commit db9de27.

RUMM-612 Revert "Run Thread sanitizer with symbolication"

25dd775

This reverts commit 54044e4.

RUMM-612 Modify URLSessionSwizzler to call end span before calling or…

6384af0

…iginal completion method Address some more PR comments

RUMM-612 Fix URLSessionSwizzlerTests, order of expectations was not c…

6eb0c78

…orrect after previous changes

RUMM-612 Fix failing test

ff64969

After fixing URLSessionSwizzler finishing spans, this spans are now root spans

nachoBonafonte force-pushed the nachoBonafonte/RUMM-612-Add-active-span-concept-to-Tracer branch from 8536551 to ff64969 Compare August 5, 2020 12:44

ncreated approved these changes Aug 5, 2020

View reviewed changes

nachoBonafonte merged commit a240fc2 into master Aug 5, 2020

nachoBonafonte deleted the nachoBonafonte/RUMM-612-Add-active-span-concept-to-Tracer branch August 5, 2020 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUMM-612 Add active span concept to Tracer #187

RUMM-612 Add active span concept to Tracer #187

nachoBonafonte commented Jul 17, 2020

buranmert left a comment •

edited

Loading

ncreated left a comment

nachoBonafonte commented Jul 17, 2020

ncreated commented Jul 17, 2020

nachoBonafonte commented Aug 4, 2020

nachoBonafonte commented Aug 4, 2020

ncreated commented Aug 4, 2020

ncreated commented Aug 4, 2020

nachoBonafonte commented Aug 4, 2020

buranmert commented Aug 4, 2020

ncreated left a comment •

edited by nachoBonafonte

Loading

ncreated commented Aug 5, 2020

buranmert commented Aug 5, 2020

nachoBonafonte commented Aug 5, 2020

nachoBonafonte commented Aug 5, 2020 •

edited

Loading

ncreated left a comment

RUMM-612 Add active span concept to Tracer #187

RUMM-612 Add active span concept to Tracer #187

Conversation

nachoBonafonte commented Jul 17, 2020

What and why?

How?

Review checklist

buranmert left a comment • edited Loading

Choose a reason for hiding this comment

ncreated left a comment

Choose a reason for hiding this comment

Flakiness

os.activity APIs

Backward compatibility

All platform tests

nachoBonafonte commented Jul 17, 2020

ncreated commented Jul 17, 2020

nachoBonafonte commented Aug 4, 2020

nachoBonafonte commented Aug 4, 2020

ncreated commented Aug 4, 2020

ncreated commented Aug 4, 2020

nachoBonafonte commented Aug 4, 2020

buranmert commented Aug 4, 2020

ncreated left a comment • edited by nachoBonafonte Loading

Choose a reason for hiding this comment

ncreated commented Aug 5, 2020

buranmert commented Aug 5, 2020

nachoBonafonte commented Aug 5, 2020

nachoBonafonte commented Aug 5, 2020 • edited Loading

ncreated left a comment

Choose a reason for hiding this comment

buranmert left a comment •

edited

Loading

`os.activity` APIs

ncreated left a comment •

edited by nachoBonafonte

Loading

nachoBonafonte commented Aug 5, 2020 •

edited

Loading