ref: Add hub.startTransaction extension #2626
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moving the check to the func where it matters eliminates the need for the internal field _isTransaction and the downstream logic in startSpan.
Shorter message equals smaller bundle size.
Limit ourselves to a single warning line.
The fix to eliminate the warning is to call
startTransaction({name: "some name"})
so the setName advice is misplaced.
Extract code that starts transactions from startSpan and deprecate startSpan. Move check of empty transaction name to right before sending the transaction, because it is valid to start the transaction without a name and set the name later with Transaction.setName().
HazAT
requested changes
May 29, 2020
There was a problem hiding this comment.
It's missing the point of the previous PR https://github.com/getsentry/sentry-javascript/pull/2625/files#diff-8ae4c12e7fb40f73c4ff34d16c2d0ed3R34-R39
packages/apm/src/hubextensions.ts
Outdated
| logger.warn('Will fall back to <unlabeled transaction>, use `transaction.setName()` to change it.'); | ||
| (context as TransactionContext).name = '<unlabeled transaction>'; | ||
| } | ||
| logger.warn('Deprecated: Use startTransaction to start transactions and Transaction.startChild to start spans.'); |
There was a problem hiding this comment.
We should only log this in case of an transaction
kamilogorek
reviewed
Jun 2, 2020
| * with the transaction poperty set. | ||
| */ | ||
| if ((context as any).transaction !== undefined) { | ||
| logger.warn(`Use \`Sentry.startTransaction({name: ${(context as any).transaction}})\` to start a Transaction.`); |
There was a problem hiding this comment.
That's a confusing syntax :3
HazAT
changed the title
kamilogorek
approved these changes
Jun 2, 2020
rhcarvalho
added a commit
that referenced
this pull request
Jun 8, 2020
Follow up on #2626. Now that startTransaction exists, replace usages of startSpan where the intention is to create a transaction. Additionally, documentation updates and some refactoring to minimize type castings and linter flag usage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This probably replaces #2625.
Transaction._isTransactionbecause the only way this was used is equivalent to callingHub.startTransaction.Sentry.startTransactionorHub.startTransaction, and to start spans callTransaction.startChild.