Add Spring Integration. #539
Conversation
Many of beans previously instantiated in Spring Boot integration move to Spring integration. Spring Boot integration enables more fine grained SentryOptions configuration, simple way to register custom event processors, passing git commit id as the event release version.
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
| webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT, | ||
| properties = ["sentry.dsn=http://key@localhost/proj", "sentry.send-default-pii=true"] | ||
| ) | ||
| class SentrySpringIntegrationTest { |
There was a problem hiding this comment.
This test class is almost a duplicate from Sentry Spring Boot integration. I am not sure yet if/how we can reduce the duplication keeping the same confidence level.
There was a problem hiding this comment.
should we maybe add a comment to this class and the "duplicated" one about each other? so when touching either this one or the other one, we know that they need to keep kinda synced
|
This pull request introduces 1 alert when merging 0bee84f into cd6f526 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging cd37677 into cd6f526 - view on LGTM.com new alerts:
|
sentry-samples/sentry-samples-spring-boot/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
sentry-samples/sentry-samples-spring-boot/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
...ples/sentry-samples-spring/src/main/java/io/sentry/samples/spring/SentryDemoApplication.java
Outdated
Show resolved
Hide resolved
| public class CustomEventProcessor implements EventProcessor { | ||
| @Override | ||
| public SentryEvent process(SentryEvent event, @Nullable Object hint) { | ||
| event.setTag("Java Version", System.getProperty("java.version")); |
There was a problem hiding this comment.
This is nice, should we add to the Runtime context?
https://develop.sentry.dev/sdk/event-payloads/contexts#runtime-context
Shows up like this in Sentry:
Can we get for example: JVM version, or if it's Oracle JVM vs OpenJVM?
Graal or whatever else?
We can add the logos etc.
There was a problem hiding this comment.
yep we could, but then i guess we would add it to sentry-core? I we go for it i need another idea for reasonable example of event processor ;-)
There was a problem hiding this comment.
not really, it should be a processor specifically for java backend/desktop, it should not be executed on Android at least, it won't return anything.
I mean it could live in sentry-core cus there's no compiling error, but I don't think it should be executed on runtime by default on all the platforms.
There was a problem hiding this comment.
Sentry does not recognize Java runtime. I keep it for now in a sample, we can consider moving it to sentry-core once it's agreed what runtime values are recognized.
There was a problem hiding this comment.
@maciejwalkowiak what do you mean by "Sentry does not recognize Java runtime"? if it's about the icon, this is just an UI limitation.
but agreed, we can make this in other PR, it doesn't make sense to block this one.
There was a problem hiding this comment.
Ah my bad. I thought unknown values are not passed but I just had NPE thrown in the processor. All good now :-) Anyway, I would prefer to close this one and move it to the next PR.
sentry-samples/sentry-samples-spring/src/main/resources/application.properties
Outdated
Show resolved
Hide resolved
| @@ -29,11 +31,23 @@ protected void doFilterInternal( | |||
| final @NotNull FilterChain filterChain) | |||
| throws ServletException, IOException { | |||
| hub.pushScope(); | |||
| // clears breadcrumbs that may have been added during application startup through one of the logging integrations. | |||
| hub.clearBreadcrumbs(); | |||
There was a problem hiding this comment.
would it make sense to expose the clear method in the hub then? which is basically cleaning everything from the scope?
There was a problem hiding this comment.
could we lose useful information when cleaning all the breadcrumbs?
There was a problem hiding this comment.
This is there to avoid having startup logs attached to every event sent. If we do not clear breadcrumbs every event looks like:
There was a problem hiding this comment.
IMO this is fine, not sure how noisy this can get though.
every event on android also has many startup breadcrumbs, but I believe that this is what breadcrumbs stand for, if it's like less than 5, all good, it'd be a problem if its a huge number and just trash.
maybe @bruno-garcia has opinions
I'm just afraid of losing useful breadcrumbs added by the user manually.
There was a problem hiding this comment.
I still believe these breadcrumbs create noise, but since both @marandaneto and @bruno-garcia are more leaning towards keeping them, they are now not removed.
Currently there is no option to set this on the runtime as Sentry does not recognize Java runtime.
| import org.springframework.beans.BeansException; | ||
| import org.springframework.beans.factory.config.BeanPostProcessor; | ||
|
|
||
| /** Initializes Sentry after all beans are registered. */ |
There was a problem hiding this comment.
out of curiosity, if there's a bug on customer's beans and Sentry is the last to be inited, would that mean that we won't be able to capture the crash? should it not be the other way around, as the 1st one? if possible, of course.
There was a problem hiding this comment.
This is why logging framework integration should be used as well. Logging framework is initialized first so it will catch any bug that happens during startup.
There was a problem hiding this comment.
ok sounds good, still curious how this would work, if a logging framework forwards the call to sentry, sentry won't be init./enabled yet, right, unless the logging framework has a sort of caching, not sure.
we can discuss this offline though, I'd like to understand a little bit more :) thanks.
There was a problem hiding this comment.
Logging frameworks calls init first, sends startup events to sentry, then Spring initializes Sentry and configuration provided there is used from now on. The drawback of this solution is that in order to get startup events sent to sentry DSN has to be provided twice - once to Logback configuration and then to Spring configuration.
There was a problem hiding this comment.
I see, but it's a good trade-off I'd say, we just document it properly, and it's fine IMO.
in the future we could look into it to provide a single DSN, might be possible with a little more work, just guessing here though.
There was a problem hiding this comment.
Yes, it will be possible if we follow up on #536. To get it working SDK has to have a way to obtain configuration from external source - independent from framework-specific configuration.
I think we will release what we have and then find out next steps from users feedback.
There was a problem hiding this comment.
that sounds great, thanks for explaining!
|
Thanks @marandaneto! |
|
@bruno-garcia take another look please. |
|
|
||
| @Override | ||
| public SentryEvent process(SentryEvent event, @Nullable Object hint) { | ||
| final SentryRuntime runtime = new SentryRuntime(); |
There was a problem hiding this comment.
Would be nice to do this to all events (put it in the main event processor)
Likely not going to work for Android though
There was a problem hiding this comment.
yeah it won't, we could have the processor on sentry-core but either it gets added to the processor list only if its non-Android or we do a conditional flag.
| testImplementation(Config.TestLibs.springBootStarterTest) | ||
| testImplementation(Config.TestLibs.springBootStarterWeb) | ||
| testImplementation(Config.TestLibs.springBootStarterSecurity) | ||
| testImplementation(Config.Libs.springBootStarterTest) |
There was a problem hiding this comment.
I think this one was supposed to stay as TestLibs
| testImplementation(Config.TestLibs.kotlinTestJunit) | ||
| testImplementation(Config.TestLibs.mockitoKotlin) | ||
| testImplementation(Config.Libs.springBootStarterTest) | ||
| testImplementation(Config.Libs.springBootStarterWeb) |
There was a problem hiding this comment.
if it's used in the samples, it's fine to be in the Libs IMO.
📢 Type of change
📜 Description
Many of beans previously instantiated in Spring Boot integration move to Spring integration. Spring Boot integration enables more fine grained SentryOptions configuration, simple way to register custom event processors, passing git commit id as the event release version.
💡 Motivation and Context
Feature parity with Sentry 1.x
💚 How did you test it?
📝 Checklist
🔮 Next steps