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

Improve start transaction overloads #2964

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Oct 3, 2023

📜 Description

Remove startTransaction overloads that can be replaced by passing TransactionOptions.

💡 Motivation and Context

Closes #2754

💚 How did you test it?

📝 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

@adinauer adinauer changed the base branch from main to feat/7.0.0 October 3, 2023 11:11
@@ -793,15 +780,16 @@ public static void endSession() {
* @param name the transaction name
* @param operation the operation
* @param description the description
* @param bindToScope if transaction should be bound to scope
* @param transactionOptions options for the transaction
* @return created transaction
*/
public static @NotNull ITransaction startTransaction(
final @NotNull String name,
final @NotNull String operation,
final @Nullable String description,
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we wanna keep this overload that allows passing in a description?

Copy link
Member

@romtsn romtsn Oct 4, 2023

Choose a reason for hiding this comment

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

yep, I think I'd keep it as description is quite popular to set, and we also use it for indexing spans in starfish, so it's an important field

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

Fails
🚫 Please consider adding a changelog entry for the next release.
Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

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

- Improve start transaction overloads ([#2964](https://github.com/getsentry/sentry-java/pull/2964))

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 3efcf65

@adinauer
Copy link
Member Author

adinauer commented Oct 3, 2023

Not quite sure we wanna take on startChild on the transaction as well. We'd have to move some params onto SpanOptions which in case of instrumenter would be weird since TransactionOptions inherits it and then on startTransaction it'd be confusing for users as there'd be an instrumenter option on TransactionOptions (via SpanOptions) and also on TransactionContext (which is actually used for transactions).

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! still needs a changelog entry though

@romtsn
Copy link
Member

romtsn commented Oct 4, 2023

Not quite sure we wanna take on startChild on the transaction as well. We'd have to move some params onto SpanOptions which in case of instrumenter would be weird since TransactionOptions inherits it and then on startTransaction it'd be confusing for users as there'd be an instrumenter option on TransactionOptions (via SpanOptions) and also on TransactionContext (which is actually used for transactions).

yeah, I think it's fine to skip this one, it's also just 5 overloads there, not that bad

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 399.47 ms 470.12 ms 70.65 ms
Size 1.72 MiB 2.26 MiB 550.19 KiB

Baseline results on branch: feat/7.0.0

Startup times

Revision Plain With Sentry Diff
ff784c6 388.51 ms 461.90 ms 73.39 ms
6684058 414.76 ms 511.46 ms 96.70 ms
db8763d 386.60 ms 453.66 ms 67.06 ms

App size

Revision Plain With Sentry Diff
ff784c6 1.72 MiB 2.26 MiB 550.22 KiB
6684058 1.72 MiB 2.26 MiB 550.19 KiB
db8763d 1.72 MiB 2.26 MiB 550.22 KiB

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Files Coverage Δ
sentry/src/main/java/io/sentry/Hub.java 75.77% <ø> (-0.22%) ⬇️
sentry/src/main/java/io/sentry/HubAdapter.java 91.54% <ø> (-0.24%) ⬇️
sentry/src/main/java/io/sentry/IHub.java 95.23% <100.00%> (+3.23%) ⬆️
sentry/src/main/java/io/sentry/NoOpHub.java 46.93% <ø> (+1.84%) ⬆️
sentry/src/main/java/io/sentry/Sentry.java 87.39% <100.00%> (+0.89%) ⬆️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@adinauer adinauer merged commit 1513e7e into feat/7.0.0 Oct 6, 2023
17 checks passed
@adinauer adinauer deleted the feat/improve-start-transaction-overloads branch October 6, 2023 15:13
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.

Improve Sentry.startTransaction overloads
2 participants