-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
ref: Remove SentryTransaction from public API. #950
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a suggestion based on our last meeting about Performance API and the open discussion #936.
Thoughts?
/** | ||
*Span name. | ||
*/ | ||
@property (nonatomic, copy) NSString *name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spans don't have a name, only a Transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a suggestion based on the openTelemetry definition.
Removing the name of span creates a new "problem", startTransaction requires a name as a property, but the return is a Span, and the user cannot check the name again.
Lets say the user has a callback to define sampling, and it uses the transaction name in the decision process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this is the API design on Cocoa, because on Java we don't rely on the transaction object to do sampling, see docs https://docs.sentry.io/platforms/java/configuration/sampling/
during the call, the discussion was more around, the SentryTransaction class still exists, but we'd expose only Span, which means, internally SentryTransaction still is used (sampling would be a case I guess), the user wouldn't only manipulate it directly, this would reduce the amount of breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not work with sampling yet, but I really thought the user would have access to the span for decision making, it make more sense to me knowing what I'm sampling to decide.
Still find it odd to have a parameter and then don't have a way to access the value used.
Well, let's leave this discussion to the future if some user case appears, for now I will remove the name field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brustolin this discussion already came up, getsentry/develop#243 we do head-based sampling, so we won't likely have the full information, anyway, this is a known limitation right now, you've been tagged to that issue btw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruno-garcia we had 2 options during the call, one was just hiding SentryTransaction
but keeping it as it is, creating it, and adding spans to it, the return type would be ISpan
though, so hidden in the Public API point of view.
the other one would be creating a SentryTransaction
only at the end, as written in your code snippet, this one would require more changes (adding a root span boolean to the span? adding a name to the span? do we get start timestamp from root span? etc...).
We didn't decide on the 1st or 2nd, but actually, to Draft a PR and see what makes more sense, @maciejwalkowiak said that the 1st option would be easier/faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am reconsidering. Option 1st is also not trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR offers a third option. I had created a Tracer class (inspired in openTelemetry) that implements Span, is thread-safe, and responsible to handle spans creation. The Tracer finish has the approach mentioned by bruno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is fine but this would require a proposal & approval API (so this PR would be a Draft rather than ready to be reviewed/merged), because it differs quite a lot from other SDKs, not telling is a bad idea, but trying to align with other folks first (whom originally designed the current API), the 2 options discussed during the meeting still kept the same public API but not exposing the Transaction object.
maybe @rhcarvalho has something to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to propose a new way that will be taken by the other SDKs.
The complexity of the refactor plays a role but can't be the main driver. We need to sort out some major concerns (which this PR addresses).
Lets review this on a call collect the points so we can move forward
return self; | ||
} | ||
|
||
- (SentrySpan *)startChildWithOperation:(NSString *)operation | ||
- (id<SentrySpan>)startChildWithName:(NSString *)name operation:(NSString *)operation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brustolin could you pull master and feat/performance-monitoring into this branch, feels like a lot of changes are unrelated to this PR and hard to review (73 files) |
* 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>
📜 Description
Created a SentrySpan protocol to expose in the SDK and removed SentryTransaction from the public API.
💡 Motivation and Context
The user should not have access to SentryTransaction.
💚 How did you test it?
Unit tests
📝 Checklist
🔮 Next steps
TBD