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

High throughput transport implementation #1114

Merged
merged 15 commits into from
Jan 12, 2021
Merged

Conversation

maciejwalkowiak
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak commented Dec 15, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Provides high throughput transport implementation meant to be used by backend applications using the performance feature.

Extracts rate limiting functionality to separate class, making it possible to use from other implementation of ITransport.

💡 Motivation and Context

Fixes #1097.

  • extracted rate-limiter that can be re-used in multiple transport implementations
  • lifting TransportFactory as a top level interface that can be set on SentryOptions - to provide an easy way to set custom transports
  • Apache Http Client 5 based high throughput transport implementation that under the hood is based on non blocking IO.
  • Spring Boot auto-configuration for the new transport

The last two are in the PR pending review (#1136).

💚 How did you test it?

Unit & integration tests.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

…1118)

Flip the relation between `AsyncConnection` and `ITransport` and make `BlockingHttpTransport` use `HttpConnection`, making it possible to provide alternative transport implementations not bound to use thread pool.
@codecov-io
Copy link

Codecov Report

Merging #1114 (2081d2e) into main (ef1eef7) will decrease coverage by 0.31%.
The diff coverage is 78.46%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1114      +/-   ##
============================================
- Coverage     74.95%   74.64%   -0.32%     
+ Complexity     1612     1605       -7     
============================================
  Files           166      167       +1     
  Lines          5603     5609       +6     
  Branches        548      550       +2     
============================================
- Hits           4200     4187      -13     
- Misses         1150     1164      +14     
- Partials        253      258       +5     
Impacted Files Coverage Δ Complexity Δ
...c/main/java/io/sentry/transport/NoOpTransport.java 0.00% <0.00%> (-40.00%) 0.00 <0.00> (-2.00)
.../io/sentry/transport/QueuedThreadPoolExecutor.java 80.95% <ø> (ø) 7.00 <0.00> (ø)
...main/java/io/sentry/transport/StdoutTransport.java 77.77% <ø> (+17.77%) 2.00 <0.00> (ø)
.../src/main/java/io/sentry/NoOpTransportFactory.java 66.66% <66.66%> (ø) 2.00 <2.00> (?)
sentry/src/main/java/io/sentry/SentryOptions.java 83.90% <75.00%> (+0.06%) 106.00 <2.00> (ø)
.../main/java/io/sentry/transport/HttpConnection.java 75.00% <75.00%> (ø) 18.00 <18.00> (?)
...src/main/java/io/sentry/transport/RateLimiter.java 75.47% <75.47%> (ø) 26.00 <26.00> (?)
...n/java/io/sentry/transport/AsyncHttpTransport.java 61.85% <82.14%> (ø) 8.00 <5.00> (?)
...src/main/java/io/sentry/log4j2/SentryAppender.java 82.50% <100.00%> (ø) 19.00 <0.00> (ø)
...rc/main/java/io/sentry/logback/SentryAppender.java 86.84% <100.00%> (-1.32%) 21.00 <1.00> (-1.00)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef1eef7...2081d2e. Read the comment docs.


URL sentryUrl;
try {
sentryUrl = parsedDsn.getSentryUri().toURL();
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd do this eagerly (after init) so we throw at that point and crash.
Once the SDK inits we should capture all exceptions and log it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is done with init. If DSN is wrong SDK init will fail.

This piece will be refactored anyways in subsequent PR as every transport needs an envelope url, so there is no point for each factory to resolve it by itself.

envelopeCache.discard(envelope);
}
} else {
executor.submit(new EnvelopeSender(filteredEnvelope, hint, currentEnvelopeCache));
Copy link
Member

Choose a reason for hiding this comment

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

What happens if we capture in a tight loop, executor drops after maxItems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in such case once we reach max queue items are dropped.

@bruno-garcia
Copy link
Member

any blockers for merging this?

@maciejwalkowiak
Copy link
Contributor Author

It's a PR for branch that takes all the NIO related changes in. Once #1136 is approved, this whole feature is complete and we can merge this PR too.

@maciejwalkowiak maciejwalkowiak changed the title Extract Rate Limiter High throughput transport implementation Dec 31, 2020
@bruno-garcia bruno-garcia mentioned this pull request Jan 5, 2021
22 tasks
@maciejwalkowiak
Copy link
Contributor Author

#1136 is merged. Please do a final review of this PR.

@maciejwalkowiak maciejwalkowiak merged commit 8159439 into main Jan 12, 2021
@maciejwalkowiak maciejwalkowiak deleted the gh-1097-rate-limiter branch January 12, 2021 20:19
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.

Java Server Transport
4 participants