Skip to content
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: Performance API: Transaction #908

Merged
merged 21 commits into from
Jan 28, 2021

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Jan 18, 2021

📜 Description

Created a folder with performance API.
Hacked the hell out of the SDK to be able to send a transaction to Sentry.

image

image

image

💡 Motivation and Context

Add performance API to sentry-cocoa SDK.

💚 How did you test it?

This is a draft PR, trying it out from the Sentry iOS app, checking result in Sentry.
Test will came later.

📝 Checklist

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

🔮 Next steps

TBD

Samples/iOS-ObjectiveC/iOS-ObjectiveC/ViewController.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/ISentrySpan.h Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentrySpanContext.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.h Outdated Show resolved Hide resolved
…to feature/performance

# Conflicts:
#	Sentry.xcodeproj/project.pbxproj
@bruno-garcia
Copy link
Member

We should agree on the next steps. I guess if we can startTransaction on the Hub and then have finish on the transaction itself capture it, that could be a good milestone. So build from here to do that, plus get tests in so we can merge.

Probably best to keep a branch to merge into, and we pull master into that branch from time to time. So that we can continue to release master without having to add the incomplete performance feature.

@philipphofmann @marandaneto wdyt?

@brustolin brustolin changed the title First Transaction sent to Sentry Starting performance API Jan 18, 2021
@bruno-garcia bruno-garcia changed the title Starting performance API Performance API: Transaction Jan 18, 2021
@codecov-io
Copy link

codecov-io commented Jan 18, 2021

Codecov Report

Merging #908 (42a9cc2) into feat/performance-monitoring (4e1b2fa) will decrease coverage by 2.81%.
The diff coverage is 0.00%.

Impacted file tree graph

@@                       Coverage Diff                       @@
##           feat/performance-monitoring     #908      +/-   ##
===============================================================
- Coverage                        94.73%   91.92%   -2.82%     
===============================================================
  Files                               76       80       +4     
  Lines                             3497     3590      +93     
===============================================================
- Hits                              3313     3300      -13     
- Misses                             184      290     +106     
Impacted Files Coverage Δ
Sources/Sentry/SentryClient.m 92.00% <0.00%> (-1.41%) ⬇️
Sources/Sentry/SentryEnvelope.m 84.27% <0.00%> (-8.14%) ⬇️
Sources/Sentry/Transactions/SentrySpanContext.m 0.00% <0.00%> (ø)
Sources/Sentry/Transactions/SentrySpanId.m 0.00% <0.00%> (ø)
Sources/Sentry/Transactions/SentryTransaction.m 0.00% <0.00%> (ø)
...ces/Sentry/Transactions/SentryTransactionContext.m 0.00% <0.00%> (ø)
Sources/Sentry/SentrySessionTracker.m 100.00% <0.00%> (ø)
... and 1 more

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 4e1b2fa...45eedf9. Read the comment docs.

@bruno-garcia bruno-garcia changed the base branch from master to feat/performance-monitoring January 18, 2021 22:17
@bruno-garcia
Copy link
Member

I changed the base branch to feat/performance-monitoring, which can be used for all PRs related to performance. Draft PR for that branch for visibility: #909

@philipphofmann philipphofmann changed the title Performance API: Transaction feat: Performance API: Transaction Jan 19, 2021
@marandaneto
Copy link
Contributor

We should agree on the next steps. I guess if we can startTransaction on the Hub and then have finish on the transaction itself capture it, that could be a good milestone. So build from here to do that, plus get tests in so we can merge.

Probably best to keep a branch to merge into, and we pull master into that branch from time to time. So that we can continue to release master without having to add the incomplete performance feature.

@philipphofmann @marandaneto wdyt?

that would be ideal

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice hacking. I'm excited to see some transactions in our SDK. 🥳

I know this is a draft PR, but still, I made lots of comments, so we can get this PR ready for review.

Please move all public headers into the Sources/Public folder see https://github.com/getsentry/sentry-cocoa/blob/master/CONTRIBUTING.md#public-headers. Please also move all .m files to Sources/Sentry.

I'm looking forward to seeing some tests for all of this.

Sources/Sentry/SentryEnvelope.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryEnvelope.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/ISentrySpan.h Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/ISentrySpan.h Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentrySpanContext.h Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.h Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.m Outdated Show resolved Hide resolved
Sources/Sentry/Transactions/SentryTransaction.h Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

marandaneto commented Jan 19, 2021

@brustolin just to explain the comments prefixes:

l: low, that means, its not important for the reviewer, you could ignore but it'd be nice to be fixed/replied.
m: mid, it's sort of nice that you both agree either on a solution or to ignore it.
h; high, its really important for the reviewer, you definitely should look into it/or reason why not.

context: https://blog.danlew.net/2020/04/15/the-logaf-scale/

@brustolin
Copy link
Contributor Author

I think I fixed everything you guys commented.

By Bruno request I changed SentryTransaction inheritance to SentryEvent and remove some of the previous SentryTransaction handlers like "sendTransaction" from SentryClient to use "sendEvent", this way the client can prepare the envelope with all required information.

SentryTransaction tests included

@marandaneto
Copy link
Contributor

I think I fixed everything you guys commented.

By Bruno request I changed SentryTransaction inheritance to SentryEvent and remove some of the previous SentryTransaction handlers like "sendTransaction" from SentryClient to use "sendEvent", this way the client can prepare the envelope with all required information.

SentryTransaction tests included

nice, thanks for that.

I've written a few more comments.
btw please resolve the comments that are already addressed, it's easier to review :)

brustolin and others added 2 commits January 27, 2021 11:02
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see some tests already. Is this still draft?
What's missing to get merged other than address open review comments?

Sources/Sentry/Public/SentryClient.h Show resolved Hide resolved
Sources/Sentry/Public/SentryHub.h Show resolved Hide resolved
Sources/Sentry/SentryClient.m Show resolved Hide resolved
@brustolin
Copy link
Contributor Author

brustolin commented Jan 28, 2021

Well, now I think I have addressed all comments. Still figuring out this way of using GitHub.
A couple of discussion to be solved yet, but I think is ready to be merged, and let's solve these bigger issues in future PRs.

@brustolin brustolin marked this pull request as ready for review January 28, 2021 00:41
@brustolin brustolin requested a review from HazAT as a code owner January 28, 2021 00:41
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets just address the last open issues pending replies from folks in Vienna but other than those points, LGTM

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brustolin I believe we agreed on one point, pass nil as scope. Other than that I think we can merge for now. The Transaction/Event discussion we can address later at least, lets not block this PR on that

Sources/Sentry/Public/SentryClient.h Show resolved Hide resolved
@bruno-garcia bruno-garcia merged commit b59b0ba into feat/performance-monitoring Jan 28, 2021
@bruno-garcia bruno-garcia deleted the feature/performance branch January 28, 2021 23:30
philipphofmann added a commit that referenced this pull request Jan 29, 2021
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>
philipphofmann added a commit that referenced this pull request Jan 29, 2021
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>
philipphofmann added a commit that referenced this pull request Feb 2, 2021
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>
bruno-garcia added a commit that referenced this pull request Mar 11, 2021
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants