Skip to content

Make RequestDetailsResolver public #4326

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

milanboers
Copy link

📜 Description

Follow up to #4323

This makes RequestDetailsResolver public and adds a constructor which just takes the a DSN and client name.

💡 Motivation and Context

ITransportFactory.create currently receives a RequestDetails. This means that, when creating a custom transport, you can't change the request details (dsn, auth header, ..) without resorting to ugly rewrites on the url / headers.

Having a public RequestDetailsResolver makes it easy to make a new instance with the desired DSN / client name.

Use case (from #4323) is to make a transport that can send to multiple different DSNs. (Somewhat simplified) example usage for that:

record MultiplexedTransportFactory(List<String> dsns) implements ITransportFactory {
  @Override
  public ITransport create(final SentryOptions options, final RequestDetails requestDetails) {
    final var transports = dsns.stream()
        .map(dsn -> new RequestDetailsResolver(dsn, options.getSentryClientName()).resolve())
        .map(newRequestDetails -> new AsyncHttpTransportFactory().create(options, newRequestDetails))
        .toList();
    return new ITransport() {
      @Override
      public void send(SentryEnvelope envelope, Hint hint) throws IOException {
        for (ITransport transport : transports) {
          transport.send(envelope, hint);
        }
      }

      @Override
      public boolean isHealthy() {
        return transports.stream().allMatch(ITransport::isHealthy);
      }

      @Override
      public void flush(long timeoutMillis) {
        transports.forEach(transport -> transport.flush(timeoutMillis));
      }

      @Override
      public RateLimiter getRateLimiter() {
        return null;
      }

      @Override
      public void close(boolean isRestarting) throws IOException {
        for (ITransport transport : transports) {
          transport.close(isRestarting);
        }
      }

      @Override
      public void close() throws IOException {
        for (ITransport transport : transports) {
          transport.close();
        }
      }
    };
  }
}

💚 How did you test it?

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@milanboers milanboers force-pushed the milanboers/public-request-resolver branch from 1eaf01c to 2a3820d Compare April 10, 2025 12:21
@milanboers milanboers force-pushed the milanboers/public-request-resolver branch from 2a3820d to f17822c Compare April 10, 2025 12:32

public RequestDetailsResolver(
final @NotNull String dsn, final @Nullable String sentryClientName) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could technically do without this constructor, make the SentryOptions one public, and pass fake SentryOptions with only dsn and client name from the caller side. Disadvantage is it would break if any other options are used in the future. (but disadvantage of this constructor is that adding a parameter would be breaking API change)

@milanboers milanboers marked this pull request as ready for review April 10, 2025 12:37
@lcian
Copy link
Member

lcian commented Apr 10, 2025

Thanks @milanboers.
Yeah in theory you could do this by using a fake SentryOptions but I think it's better to have a new constructor.
You could also circumvent the fact that the class is not public by defining your custom transport class in the io.sentry package.
I'll check with the team if we want to make this a public API or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants