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 monitoring #909

Merged
merged 18 commits into from
Mar 11, 2021
Merged

Conversation

bruno-garcia
Copy link
Member

This PR is not going to receive commits directly. It'll serve as base for PRs related to performance monitoring and will be merged into master without squash.

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

let's also pull master into feat/performance-monitoring sometimes so we resolve smaller junks of conflicts if any

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #909 (8284893) into master (ab03268) will decrease coverage by 0.13%.
The diff coverage is 92.99%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
- Coverage   95.21%   95.08%   -0.14%     
==========================================
  Files          79       88       +9     
  Lines        3577     3787     +210     
==========================================
+ Hits         3406     3601     +195     
- Misses        171      186      +15     
Impacted Files Coverage Δ
Sources/Sentry/SentryClient.m 93.92% <ø> (ø)
Sources/Sentry/SentrySDK.m 88.09% <0.00%> (-2.15%) ⬇️
Sources/Sentry/SentryTransactionContext.m 33.33% <33.33%> (ø)
Sources/Sentry/TracesSampler.m 88.88% <88.88%> (ø)
Sources/Sentry/SentryTracer.m 94.73% <94.73%> (ø)
Sources/Sentry/SentryTransaction.m 94.73% <94.73%> (ø)
Sources/Sentry/Random.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryEnvelope.m 93.24% <100.00%> (+0.18%) ⬆️
Sources/Sentry/SentryHub.m 98.02% <100.00%> (+0.10%) ⬆️
Sources/Sentry/SentryOptions.m 99.15% <100.00%> (+0.05%) ⬆️
... and 13 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 ab03268...8284893. Read the comment docs.

@philipphofmann
Copy link
Member

I configured Github Actions with e08c892 for this branch.

@bruno-garcia
Copy link
Member Author

let's also pull master into feat/performance-monitoring sometimes so we resolve smaller junks of conflicts if any

Yes but this time maybe rebase, so we dont' end up with a lot of merge commits into main when we merge there without squash

@philipphofmann philipphofmann force-pushed the feat/performance-monitoring branch 3 times, most recently from 8830e88 to 436c1b7 Compare January 29, 2021 07:47
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.

I know this is WIP, and we want to see something happening, but before merging this PR, let's add tests for the new functionality. Codecov tells us the coverage is going down.

bruno-garcia and others added 5 commits February 2, 2021 12:18
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>
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@philipphofmann
Copy link
Member

This PR is not going to receive commits directly. It'll serve as base for PRs related to performance monitoring and will be merged into master without squash.

What about squashing this whole PR as just one commit Performance API to master? So we can just merge like crazy to this PR and don't have to rebase all the time? @bruno-garcia @marandaneto and @brustolin

@marandaneto
Copy link
Contributor

This PR is not going to receive commits directly. It'll serve as base for PRs related to performance monitoring and will be merged into master without squash.

What about squashing this whole PR as just one commit Performance API to master? So we can just merge like crazy to this PR and don't have to rebase all the time? @bruno-garcia @marandaneto and @brustolin

I prefer not squashing, history might be valuable when investigating things

brustolin and others added 3 commits February 11, 2021 18:37
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>
Co-authored-by: Sentry Github Bot <bot+github-bot@sentry.io>
@philipphofmann
Copy link
Member

What about merging this to master, as we have 7.0.0-alpha.0 already? @bruno-garcia, @marandaneto

@marandaneto
Copy link
Contributor

What about merging this to master, as we have 7.0.0-alpha.0 already? @bruno-garcia, @marandaneto

there are still a few moving bits, I'd wait for the sampling context PR to be merged and work a bit on samples.
right now the samples create a transaction, start an async dispatch, and finish the transaction.

I'd prefer to have a sample that creates a transaction, start a child, finish as errored, capture an event in-between, see if it associate its data, etc, right now there are only unit tests but no real usage/similar to the reality to be sure that everything renders nicely in the UI, other than that, I'd be totally in for making an alpha, what do you think @bruno-garcia @philipphofmann @brustolin ?

* 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>
@bruno-garcia
Copy link
Member Author

bruno-garcia commented Mar 2, 2021

What about merging this to master, as we have 7.0.0-alpha.0 already? @bruno-garcia, @marandaneto

there are still a few moving bits, I'd wait for the sampling context PR to be merged and work a bit on samples.
right now the samples create a transaction, start an async dispatch, and finish the transaction.

I'd prefer to have a sample that creates a transaction, start a child, finish as errored, capture an event in-between, see if it associate its data, etc, right now there are only unit tests but no real usage/similar to the reality to be sure that everything renders nicely in the UI, other than that, I'd be totally in for making an alpha, what do you think @bruno-garcia @philipphofmann @brustolin ?

Makes total sense to me! Sample would be great.

#970

brustolin and others added 2 commits March 8, 2021 10:32
* 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>
@bruno-garcia bruno-garcia marked this pull request as ready for review March 8, 2021 23:44
brustolin and others added 2 commits March 8, 2021 20:54
* 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>
@marandaneto
Copy link
Contributor

@brustolin please resolve the open comments if they are already resolved, so it's easier to review it again, thanks

@marandaneto
Copy link
Contributor

changelog requires merging

@marandaneto
Copy link
Contributor

@bruno-garcia can we check if the sampling decision follows the correct format based on the discussion yesterday? I quite didn't fully get it yet and it'd be nice to double-check this before releasing, even if it's wrong, it'd be an easy fix.

@philipphofmann
Copy link
Member

@bruno-garcia can we check if the sampling decision follows the correct format based on the discussion yesterday? I quite didn't fully get it yet and it'd be nice to double-check this before releasing, even if it's wrong, it'd be an easy fix.

@marandaneto, I created an issue for this #986. I think it's fine to release the alpha without this fix.

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.

For an alpha LGTM. I don't want to keep this on an extra branch any longer. Is there anything stopping us from merging this @bruno-garcia, @marandaneto, and @brustolin?

@marandaneto
Copy link
Contributor

@bruno-garcia can we check if the sampling decision follows the correct format based on the discussion yesterday? I quite didn't fully get it yet and it'd be nice to double-check this before releasing, even if it's wrong, it'd be an easy fix.

@marandaneto, I created an issue for this #986. I think it's fine to release the alpha without this fix.

it's not about that, it's something else.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@bruno-garcia bruno-garcia merged commit fff097c into master Mar 11, 2021
@bruno-garcia bruno-garcia deleted the feat/performance-monitoring branch March 11, 2021 20:30
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