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

Proposed new performance API (runTransaction) #1154

Open
bruno-garcia opened this issue Jan 8, 2021 · 9 comments
Open

Proposed new performance API (runTransaction) #1154

bruno-garcia opened this issue Jan 8, 2021 · 9 comments
Labels
enhancement New feature or request performance Performance API issues Platform: Java

Comments

@bruno-garcia
Copy link
Member

bruno-garcia commented Jan 8, 2021

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: #1151 (comment)

First usage of it: #1058 (comment)

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.

@bruno-garcia bruno-garcia added the enhancement New feature or request label Jan 8, 2021
@marandaneto
Copy link
Contributor

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

@marandaneto
Copy link
Contributor

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
      }

@maciejwalkowiak
Copy link
Contributor

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?

@marandaneto
Copy link
Contributor

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 :(

@untitaker
Copy link
Member

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

@bruno-garcia
Copy link
Member Author

This relates to the the .NET version of it: getsentry/sentry-dotnet#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)

@bruno-garcia
Copy link
Member Author

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.

@marandaneto
Copy link
Contributor

marandaneto commented Feb 15, 2021

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

@adinauer
Copy link
Member

adinauer commented May 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance Performance API issues Platform: Java
Projects
Status: Backlog
Development

No branches or pull requests

6 participants