Skip to content

fix: Keep status of auto transactions when finishing #2684

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

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Feb 8, 2023

📜 Description

Don't overwrite the existing status of auto generated transactions when finishing them.

💡 Motivation and Context

Fixes GH-2402

💚 How did you test it?

Unit tests

Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
philipphofmann Philipp Hofmann
Don't overwrite the existing status of auto generated transactions
when finishing them.

Fixes GH-2402
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1232.02 ms 1247.94 ms 15.92 ms
Size 20.76 KiB 420.72 KiB 399.96 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
5cabf7e 1247.12 ms 1249.61 ms 2.49 ms
ecd9ecd 1204.67 ms 1226.46 ms 21.79 ms
9faf217 1268.86 ms 1274.82 ms 5.96 ms
bd2afa6 1192.31 ms 1210.37 ms 18.05 ms
8f397a7 1236.76 ms 1256.76 ms 20.00 ms
ecd9ecd 1215.77 ms 1238.70 ms 22.93 ms
156e771 1228.06 ms 1242.64 ms 14.58 ms
8f397a7 1196.55 ms 1226.82 ms 30.27 ms
83d2d84 1211.31 ms 1227.34 ms 16.03 ms
8f397a7 1251.82 ms 1268.34 ms 16.52 ms

App size

Revision Plain With Sentry Diff
5cabf7e 20.76 KiB 419.67 KiB 398.91 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB
9faf217 20.76 KiB 419.70 KiB 398.94 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
ecd9ecd 20.76 KiB 420.23 KiB 399.47 KiB
156e771 20.76 KiB 419.70 KiB 398.94 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
83d2d84 20.76 KiB 419.66 KiB 398.90 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB

Previous results on branch: fix/keep-status-auto-transactions

Startup times

Revision Plain With Sentry Diff
ec5adf7 1255.59 ms 1268.80 ms 13.21 ms

App size

Revision Plain With Sentry Diff
ec5adf7 20.76 KiB 420.59 KiB 399.84 KiB

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #2684 (d7ccab5) into main (bd2afa6) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head d7ccab5 differs from pull request most recent head 0fd882f. Consider uploading reports for the commit 0fd882f to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2684      +/-   ##
==========================================
+ Coverage   80.52%   80.57%   +0.05%     
==========================================
  Files         247      247              
  Lines       22862    22877      +15     
  Branches    10121    10127       +6     
==========================================
+ Hits        18409    18433      +24     
+ Misses       3990     3983       -7     
+ Partials      463      461       -2     
Impacted Files Coverage Δ
Sources/Sentry/SentryTracer.m 97.45% <100.00%> (+0.01%) ⬆️
Sources/Sentry/SentryRequestOperation.m 67.56% <0.00%> (-10.82%) ⬇️
Sources/Sentry/SentryCrashIntegration.m 98.66% <0.00%> (+0.03%) ⬆️
Sources/Sentry/SentryNetworkTracker.m 95.28% <0.00%> (+0.08%) ⬆️
Sources/Sentry/SentrySerialization.m 87.97% <0.00%> (+0.12%) ⬆️
Sources/Sentry/SentryFileManager.m 95.67% <0.00%> (+0.61%) ⬆️
Sources/Sentry/SentryThreadInspector.m 98.00% <0.00%> (+2.00%) ⬆️
Sources/SentryCrash/Recording/Tools/fishhook.c 82.27% <0.00%> (+5.06%) ⬆️

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 bd2afa6...0fd882f. Read the comment docs.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
philipphofmann Philipp Hofmann
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@philipphofmann philipphofmann merged commit d413317 into main Feb 14, 2023
@philipphofmann philipphofmann deleted the fix/keep-status-auto-transactions branch February 14, 2023 14:39
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Keep status of auto transactions when finishing ([#2684](https://github.com/getsentry/sentry-cocoa/pull/2684))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against bcd75a8

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.

Do not overwrite existing status of UI event transactions
2 participants