-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Add instrumenter
option
#2349
Add instrumenter
option
#2349
Conversation
|
Performance metrics 🚀
|
@@ -20,7 +20,10 @@ public interface ISpan { | |||
@ApiStatus.Internal | |||
@NotNull | |||
ISpan startChild( | |||
@NotNull String operation, @Nullable String description, @Nullable Date timestamp); | |||
@NotNull String 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.
if you break this api, won't you have to update every call site of span.startChild
?
The way I did in ruby was to have an optional arg instrumenter
that defaults to sentry
.
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.
it's marked as internal and is only used in a couple of places by ourselves, but I agree, we should probably add a new method overload here
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 was explicitly trying to avoid more overloads since this was only used in very few places and it felt cleaner to just add the new param in those places. I did update the call sites. Unfortunately no default params in Java 😢 .
@romtsn you think we need the overload or can we proceed with the changed method signature? At some point we might want to introduce some sort of param object as we did with startTransaction
and TransactionOptions
to handle defaults etc.
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 can proceed I guess
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.
Should we also add a test for SentryTracer
changes?
@romtsn These are tested via |
ah I missed that, that should be fine then |
#skip-changelog
📜 Description
This makes
startTransaction
andstartChild
no-op if the instrumenter starting the operation doesn't match the configured one.💡 Motivation and Context
To avoid double instrumenting when OTEL is being used.
💚 How did you test it?
📝 Checklist
🔮 Next steps