feat: Adding SentrySpan to performance monitoring. by brustolin · Pull Request #932 · getsentry/sentry-cocoa

brustolin · 2021-02-03T02:32:47Z

📜 Description

Creating the SentrySpan class and allowing transactions to create child.

💡 Motivation and Context

Requisite to the Performance API.

💚 How did you test it?

Unit tests

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the CHANGELOG if needed
I updated the docs if needed
Review from the native team if needed
No breaking changes

🔮 Next steps

TBD

bruno-garcia

quick pass, couple nits

…-cocoa into feat/sentryspan

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

philipphofmann

A first glance. I didn't have enough time to look at all of it. I will do that tomorrow.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

codecov-io · 2021-02-05T01:57:58Z

Codecov Report

Merging #932 (5dd6d12) into feat/performance-monitoring (1195ec0) will decrease coverage by 1.07%.
The diff coverage is 16.00%.

@@                       Coverage Diff                       @@
##           feat/performance-monitoring     #932      +/-   ##
===============================================================
- Coverage                        94.43%   93.36%   -1.08%     
===============================================================
  Files                               80       81       +1     
  Lines                             3612     3660      +48     
===============================================================
+ Hits                              3411     3417       +6     
- Misses                             201      243      +42

Impacted Files	Coverage Δ
Sources/Sentry/SentrySpan.m	`0.00% <0.00%> (ø)`
Sources/Sentry/SentryTransaction.m	`77.08% <31.25%> (-22.92%)`	⬇️
Sources/Sentry/SentrySpanContext.m	`85.00% <33.33%> (-15.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1195ec0...2117314. Read the comment docs.

philipphofmann

The code looks already good. I addressed many things and also took a closer look at the tests this time. There are many minor comments.

philipphofmann · 2021-02-05T09:03:19Z

+    NSMutableArray *spans = [[NSMutableArray alloc] init];
+    for (SentrySpan *span in self.spans) {
+        [spans addObject:[span serialize]];
+    }
+    serializedData[@"spans"] = spans;


h: We need to make that thread-safe as well.

It's true that this needs to be thread safe but it comes back to the fact that Transaction is an event, which isn't (and will never be) thread-safe. So we need to discuss what we do with this.

Lets discuss this Friday please, @mitsuhiko will be there

Thursday though

philipphofmann · 2021-02-05T09:50:50Z

+    span.operation = operation;
+    span.spanDescription = description;
+    span.sampled = self.isSampled;
+    @synchronized(self.spans) {


m: Can we please add a test to make sure this is thread-safe?

One might argue "make sure it's thread-safe" isn't possible to achieve :P

One might argue "make sure it's thread-safe" isn't possible to achieve :P

Indeed. Neither for positive, nor for negative outcome.

time has shown that this answer will come, soon or later, if it was or not :D
but I agree, at least trivial thread-safe problems could be avoided with tests, so a test would be indeed, nice.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

…-cocoa into feat/sentryspan

bruno-garcia

All open comments were closed except tests for thread-safety but given SentryTransaction isn't even expected to be thread safe (It won't be bound to the scope anymore as we agreed today) it isn't relevant.

bruno-garcia · 2021-02-11T23:38:05Z

A reminder that we're merging this into a branch and not into master and we can iterate, specially after our meeting today.

philipphofmann · 2021-02-12T12:31:00Z

Thanks, @bruno-garcia, for helping out on this one. I was swamped yesterday with other stuff, so I didn't take the time for another review.

* feat: performance monitoring * build: Setup CI for performance monitoring branch * feat: Performance API: Transaction (#908) Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Clang Robot <clang-robot@sentry.io> Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> * test: Add captureTransaction to sample apps (#922) * docs: Improve code comments on transactions (#921) Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> * feat: Add missing properties to SpanContext and Transaction (#919) Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> * feat: Adding SentrySpan to performance monitoring. (#932) Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Clang Robot <clang-robot@sentry.io> * ref: Remove SentryTransaction from public API. (#950) Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io> * feat: Add transaction sampling properties to SentryOptions (#961) * Transaction sampling * Update CHANGELOG.md * Format code * Apply suggestions from code review Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> * Testing and fixes * Using NSNumber for sampler return * Update CHANGELOG.md Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> * Comment Update * Using NSNumber for tracesSampleRate Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io> Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> * feat: Transaction in Sample App (#971) * Some UI Sample * Format code * Apply suggestions from code review Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> * removing button to download image * Adding status and fixing operation value * Fixed Tests Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io> Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> * merge fix * feat: Add sampling rules to SentryHub startTransaction. (#977) * Changing sampled from bool to enum, using TracesSampler * removing name from span * SamplingTests * changelog and code format * Lint Fix * Fix tests * Update Samples/iOS-Swift/iOS-Swift/TraceTestViewController.swift Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> * Random generator for sampling * missing comment * remove sampler from SentryHub+TestInit.h * Apply suggestions from code review Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Bruno Garcia <bruno@brunogarcia.com> Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> * meta: Add missing breaking change to Changelog * performance changelog Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io> Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com> Co-authored-by: Clang Robot <clang-robot@sentry.io> Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com> Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io>

brustolin and others added 2 commits February 2, 2021 23:29

Added SentrySpan

08aac4e

Committing formatted code

4d32a9d

brustolin changed the title ~~feat: Adding SentrySpan~~ feat: Adding SentrySpan to performance monitoring. Feb 3, 2021

bruno-garcia reviewed Feb 4, 2021

View reviewed changes

Comment thread Sources/Sentry/Public/SentrySpan.h Outdated

Comment thread Sources/Sentry/Public/SentrySpan.h Outdated

Comment thread Sources/Sentry/Public/SentrySpan.h Outdated

brustolin and others added 4 commits February 3, 2021 23:00

Added Tests

572b63f

Merge branch 'feat/sentryspan' of https://github.com/getsentry/sentry…

689b1fe

…-cocoa into feat/sentryspan

format

e19a4e8

Apply suggestions from code review

e8fd991

Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>

brustolin marked this pull request as ready for review February 4, 2021 02:07

brustolin requested review from HazAT and philipphofmann as code owners February 4, 2021 02:07

philipphofmann reviewed Feb 4, 2021

View reviewed changes

brustolin and others added 2 commits February 4, 2021 20:40

Apply suggestions from code review

182ea4f

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>

Committing formatted code

d24f4c9

brustolin and others added 2 commits February 4, 2021 22:58

Fixing naming convention

3d87fb2

Committing formatted code

2117314