-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat: Add missing properties to SpanContext and Transaction #919
feat: Add missing properties to SpanContext and Transaction #919
Conversation
…to feature/performance # Conflicts: # Sentry.xcodeproj/project.pbxproj
…to feature/performance
…ntry-cocoa into feature/performance
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
…ntry-cocoa into feature/performance
…ntry-cocoa into feature/performance
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>
…entry/sentry-cocoa into feat/transaction-spancontext
8830e88
to
436c1b7
Compare
Codecov Report
@@ Coverage Diff @@
## feat/performance-monitoring #919 +/- ##
===============================================================
- Coverage 94.16% 94.03% -0.13%
===============================================================
Files 80 80
Lines 3582 3607 +25
===============================================================
+ Hits 3373 3392 +19
- Misses 209 215 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome seeing performance going forward. I added a few comments.
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including all the feedback. This is getting better and better. Can we add more tests so the coverage doesn't go down, please? Keep up the improvements. 👏🏼
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the Changelog and decide which comments you still want to resolve. The most important ones are resolved. Thanks for including all the feedback. From my point of with LGTM.
{ | ||
NSMutableDictionary<NSString *, id> *serializedData = | ||
[[NSMutableDictionary alloc] initWithDictionary:[super serialize]]; | ||
serializedData[@"spans"] = @[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l
: I guess this is still empty because we still need to add spans? If so, please add a comment, so I don't wonder why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly. Spans coming soon.
352f07e
to
380bc9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already merged but just wanted to say thanks to @brustolin to work on our reviews and being resilient pointing out improvements we could do further.
* 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>
📜 Description
Adding missing properties to SentrySpanContext, and SentryTransaction properties related to the spanContext.
💡 Motivation and Context
We are aiming to complete the Performance API
💚 How did you test it?
Unit Test
📝 Checklist
🔮
Transaction span childs