search

getsentry / sentry-java

A Sentry SDK for Java, Android and other JVM languages.
https://docs.sentry.io/
MIT License
1.16k stars 435 forks source link

Performance issue - CPU Hot spot #3474

Open micopiira opened 5 months ago

micopiira commented 5 months ago

Integration

sentry-spring-jakarta

Java Version

17

Version

7.8.0

Steps to Reproduce

While profiling some reactive code using project Reactor with automatic context propagation and Sentry, I noticed quite a lot of time is spent instantiating SecureRandom objects as the SentryReactorThreadLocalAccessor.getValue call will always cause a new TracesSampler thus a new SecureRandom object to be created when hub == null || hub instanceof NoOpHub in Sentry#getCurrentHub.

SecureRandom objects are said to be thread-safe by the javadoc, would it be possible/safe to just reuse the same instance? Or reuse the TracesSampler? Or avoid cloning the NoOpHub alltogether..?

Expected Result

Not so much time spent creating SecureRandom objects

Actual Result

Screenshot of profiling results:

Screenshot 2024-06-11 at 21 17 17
adinauer commented 5 months ago

Thanks for opening this issue. Would you mind if we fix this in the upcoming version 8 of the Java SDK?

We've changed quite a lot in the SDK already but I do think this is still an issue.

As long as options don't change, we should be able to reuse the TracesSampler instance. We're planning to improve this.

micopiira commented 5 months ago

@adinauer I don't mind, it's not a blocker and we can work around this by temporarily disabling automatic context propagation. Already looking forward to the version 8 release, especially the OTEL based performance monitoring, any esimated release date?

adinauer commented 5 months ago

OK great, thanks! I'm hoping to have another alpha ready soonish, maybe next week, but don't quote me on that.

adinauer commented 5 months ago

I've opened a PR that should reduce the number of TracesSampler instances (close) to 1, depending on setup.

adinauer commented 5 months ago

Hey @micopiira, we've just released 8.0.0-alpha.2 of the Java SDK, there's instructions how to upgrade in the changelog. If you decide to give it a try, any feedback is welcome 🙏 .

adinauer commented 3 months ago

Hey @micopiira, have you had a chance to test any of the latest alpha releases?

micopiira commented 3 months ago

Hey @micopiira, have you had a chance to test any of the latest alpha releases?

I did try to setup a simple Spring Boot application that uses Micrometer Observation API that would propagate traces to Sentry through OTEL with the 8.0.0-alpha4 build. Didn't seem to get it working, but looking at your code there are still some TODO comments and some unimplemented stuff so maybe thats the reason?

See https://github.com/micopiira/sentry-micrometer-observation-otel-testing

adinauer commented 3 months ago

@micopiira, do you have more details on what didn't work? How did you start the application? Did you add the agent JAR for trying out OTEL?

micopiira commented 3 months ago

@adinauer Just didn't see any traces in Sentry, started like you would start any Spring Boot app, without any agents. So basically I'm trying to achieve:

Micrometer Observation API -> Micrometer Tracing -> Otel -> Sentry

(See the linked repo for dependencies used and the main class has all the Spring beans I registered)

Is this not a supported use case or I'm doing something wrong or just everything not implemented yet?

adinauer commented 3 months ago

Hey @micopiira, we haven't tried the Micrometer Tracing -> OTel -> Sentry path yet. I presume we'd need to provide a special dependency that makes config easier.

Are you tied to Micrometer Tracing or would using OpenTelemetry (directly) work for you as well? Are there any benefits to using Micrometer Tracing? If so, which ones?

Thanks for the sample, we can use this in the future for figuring out how to support Micrometer Tracing.

In theory if you use the Sentry Java Agent it should just forward from the Micrometer Tracing API, I'm just now sure how it'd have to be configured. The question is how do you set up Micrometer Tracing to use OpenTelemetry that is already set up (via the Agent) and just send spans into that.

Another question here is, does Micrometer expect to be hooked into OpenTelemetry similar to how Sentry does (Propagator, SpanProcessor, Exporter, Sampler, ...) and thus needs to be hooked in when setting up OpenTelemetry. That'd mean it has to be configured inside the Agent if you want the full power of auto instrumentation that OpenTelemetry has to offer. It might also mean making it work with Sentry would be more complicated.

adinauer commented 3 months ago

I forgot to mention that the original issue here should be resolved in the latest v8 alpha version regardless of using OpenTelemetry.

micopiira commented 3 months ago

Are you tied to Micrometer Tracing or would using OpenTelemetry (directly) work for you as well? Are there any benefits to using Micrometer Tracing? If so, which ones?

Well, Micrometer advertises itself as a "vendor-neutral application observability facade, Think SLF4J, but for observability.", so theres that.

And its what Spring Boot itself uses/recommends nowadays and Spring instruments itself using the Micrometer Observation API. See: https://docs.spring.io/spring-boot/reference/actuator/tracing.html So if Sentry could be used as a full OTEL exporter then any library that is already instrumented with the Micrometer Observation API would get it's traces sent to Sentry without any need for agents or any Sentry specific integrations.

And Micrometer Observation API is not just for tracing, as observations can be handled as traces, logs or metrics using ObservationHandlers so you get metrics/logs for "free" by instrumenting your code with the Observation API.

adinauer commented 3 months ago

Our idea at the moment is to integrate with OpenTelemetry more closely so Sentry uses OpenTelemetry under the hood for Performance. Just using an exporter wouldn't allow us to do this. While we might offer an endpoint to send data to directly in the future, it's not the same as having the Sentry SDK present.

We wanna take a look at also making what we have usable when using OpenTelemetry through Micrometer Tracing in the future but can't give an ETA and can't say whether it's possible.

micopiira commented 3 months ago

@adinauer Alright. I also tried to create a simple ObservationHandler, that would create Sentry traces from Micrometer Observations directly through the Sentry API, but Sentry seemed to always use the ThreadLocal current transaction as parent span which did not work out very well with reactive stuff. (This was with the v7 Sentry API)

adinauer commented 3 months ago

Is the reactive part coming from WebFlux or is it used in Micrometer Tracing? We do set the Sentry Scopes (used to be Hub) on the Context for reactor when using WebFlux and our Spring Boot integration. This should mean the correct parent can be found as it should restore the context for the reactive flow instead of simply relying on the ThreadLocal.