sentry-java

sentry-java copied to clipboard

Discuss new API to take callback and wrap the whole transaction (to be discussed on the 11th of Jan)

The proposal for this API:

Sentry.runTransaction("name", tx -> {
  // some code
});

Where runTransaction method would look like:

public static void runTransaction(String transactionName, Consumer<ITransaction> runnable) {
  ITransaction transaction = startTransaction(transactionName);
  try {
    runnable.accept(transaction);
    transaction.setStatus(SpanStatus.OK);
	return callback();
  } catch (Throwable e) {
    transaction.setThrowable(e);
    transaction.setStatus(SpanStatus.INTERNAL_ERROR);
	throw e;
  } finally {
    transaction.finish();
  }
}

With such simple API we don't give a an option to set different status on error, but it can be a good enough tradeoff.

From: https://github.com/getsentry/sentry-java/issues/1151#issuecomment-755336087

First usage of it: https://github.com/getsentry/sentry-java/pull/1058#discussion_r528037922

Downside is that we have to account for async and callbacks with return type.

Alternative

Alternative APIs to make it less boilerplate but without going full on wrapping a callback:

public static void myAppStuff() {
  ITransaction transaction = startTransaction("test");
  try {
	// whatever I do 
    transaction.finish(); // sets OK status
  } catch (Throwable e) {
    transaction.finish(e); // maps exception to status (options.ExceptionToStatusCallback)
	throw e;
  }
}

This way we can extend the mapping between Throwable and SpanStatus. Other overloads that take a HttpStatus for example could also fit well. finish parameterless would assume OK.

it'd also be nice to have something similar to runSpan or startChild offers an overload with a callback as well.

not totally related to this issue.

if we make ISpan extends AutoCloseable

we could use try-with-resources, idea from @untitaker:

      try (ITransaction tr = Sentry.startTransaction("tr")) {
          // do the work
          // tr.finish(); // not needed anymore, try with resources will do automatically
      }

try with resources is usually used when without proper closing, some resources will be kept open leaving file handles, open sockets etc open in the OS. Isn't using it for span/transaction use case a bit of misuse?

try with resources is usually used when without proper closing, some resources will be kept open leaving file handles, open sockets etc open in the OS. Isn't using it for span/transaction use case a bit of misuse?

yes, I agree, although it's been used by some other SDKs/libs to kinda reduce boilerplate, improving user experience over correctness. so I'd not use try-with-resources for the runTransaction improvement, just a parallel suggestion that could reduce boilerplate for manual instrumentation. Java 8 try-with-resources is not ideal as well, as we'd like to associate the Throwable to the current transaction/span and this is not possible as the instance is not accessible outside of the try scope. on Java 9 this would work nicely, but it does not work on Android :(

the only advantage I see with try-with-resources is that runTransaction would have to be built for sync/async functions each

This relates to the the .NET version of it: https://github.com/getsentry/sentry-dotnet/issues/801

One downside is that there's no clean way to mark the span status to failed when there's an exception (or to attach the exception to the Span)

the only advantage I see with try-with-resources is that runTransaction would have to be built for sync/async functions each

Java doesn't have async. But yeah returning Future anyway. Fwiw .NET already has ConfigureScope and ConfigureScopeAsync and such patterns is commonplace in .NET.

we agreed that such callback makes sense and reduces boilerplate for manual instrumentation.

so ideally runTransaction has the same method parameters from startTransaction but in addition an optional spanStatus so you can overwrite the default value which would be SpanStatus.INTERNAL_ERROR.

runSpan callback would also make sense, but then we'd need to either read the Span/Transaction bound to the Scope to create a child Span or create a new Transaction if not yet, this would be a bit more complex though, wondering if only being NoOp in case there's not a transaction bound to the Scope would be enough, if we decide creating a new transaction if not yet, then runTransaction kinda becomes useless as it could be fully replaced by runSpan always.

cc @Tyrrrz @brustolin

This would require TSC discussion to align SDKs and plan ahead.