OBPIH-6733 add support for Sentry performance tracing by ewaterman · Pull Request #4898 · openboxes/openboxes

ewaterman · 2024-10-15T21:17:39Z

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-6733

Description: Start publishing performance traces to Sentry. Sentry doesn't have out-of-the-box support for Grails so I needed to implement it the SpringBoot way then to get it working with Grails I had to add the transaction name to each incoming API request in a custom filter.

Here's the test project I created when testing against my local: https://openboxes.sentry.io/performance/?project=4507488767115264

And a screenshot from it showing performance traces:

…le better

ewaterman · 2024-10-18T15:38:49Z

Note that the changes here can be merged independently from the ones in my openboxes-devops PR https://github.com/openboxes/openboxes-devops/pull/74.

All we're doing in this PR is enabling performance tracing. No DSN/environment changes are happening here.

ewaterman · 2024-10-29T18:34:47Z

grails-app/conf/application.yml

-    logger: com.mysql.cj.jdbc.log.StandardLogger
    type: org.apache.tomcat.jdbc.pool.DataSource
+
+    # Settings for database logging


I just reorganized these and added some comments for clarity. You can double-check, but nothing should be changed (except formatSql: false for development).

At first it looked like you were adding new default properties, but then I realized you were just consolidating default properties that were in environment-specific blocks. So nothing to see here.

ewaterman · 2024-10-29T18:37:54Z

grails-app/conf/application.yml

-            url: jdbc:mysql://localhost:3306/openboxes?serverTimezone=UTC&useSSL=false
+            # P6Spy is an interceptor that allows us to log database queries. We use it in our Sentry integration.
+            driverClassName: com.p6spy.engine.spy.P6SpyDriver
+            url: jdbc:p6spy:mysql://localhost:3306/openboxes?serverTimezone=UTC&useSSL=false


let's discuss this. All this should be doing is adding an interceptor to capture the SQL queries, which then get forwarded on to the database as normal. Configuring it this way (via jdbc URL) is the recommended approach, though it does feel a bit weird.

Obviously we'll test it on a deployed environment but let me know if there's something that I'm missing about our setup that would prevent us from doing this.

ewaterman · 2024-10-29T18:40:57Z

@jmiranda @awalkowiak I was looking into database query logging while this was sitting in review and since I got it working I figured I'd just add it in now since I don't think either of you have had a chance to review this yet.

It's ready for review, but if possible, lets try and test this PR on a deployed environment.

ewaterman · 2024-10-30T16:21:50Z

src/main/resources/sentry.properties

+# The ratio of all requests to capture for performance tracing. 1.0 is 100% of requests. Disable tracing by setting
+# a value of 0. We keep this number low (as recommended by Sentry) for performance reasons, but it can be increased
+# if we decide we need more data.
+traces-sample-rate=0.1


this is the setting I was talking about in the tech huddle today with Andy. We only capture 10% of requests. We can adjust this as needed if we're seeing (or not seeing) a performance impact

Can we adjust this via Ansible? I assume this file is only available post-deployment, so unless we have a way of making it overridable from openboxes.yml or by adding a sentry.properties override there. Anyway, there might be something that needs to change to make this work i.e. the config locations property defined in openboxes.yml.

Good point. 10% is likely a safe bet, but it would definitely be useful to be able to configure this per environment and on the fly.

We could probably set it via environment variable with something like traces-sample-rate=${SENTRY_SAMPLE_RATE:0.1}

But I also want to take a look soon at why we're unable to set sentry properties in openboxes.yml. I totally agree that we should give people the option to configure their environments however they want.

Yes, that would be awesome. Either setting it in openboxes.yml and/or as a environment variable that openboxes.yml will use to override it's default.

But this is not a concern for the PR.

ewaterman · 2024-10-31T20:30:47Z

@jmiranda @awalkowiak based on discussions in https://openboxes.slack.com/archives/C06JCHWR807/p1730398659510229 I disabled p6spy logging by default. It can be enabled on any environment by setting the following:

dataSource:
    driverClassName: com.p6spy.engine.spy.P6SpyDriver
    url: jdbc:p6spy:mysql://localhost:3306/openboxes?serverTimezone=UTC&useSSL=false

which I'll document somewhere (along with any other monitoring info).

I'd like to take some time in the future to try to figure out a more dynamic approach. Maybe a bean that gets created like this:

DataSource getDataSource() {
    DataSource dataSource = DataSourceBuilder.create().build()
    return Sentry.enabled ? new P6DataSource(dataSource) : dataSource
}

which will auto-configure p6spy if sentry is enabled, but I'll save that for another time. I spent too long as is trying to get it to work.

jmiranda · 2024-10-31T20:44:35Z

which I'll document somewhere (along with any other monitoring info).

There's a Configure > Sentry page that needs some love.
https://docs.openboxes.com/en/obpih-6197/admin-guide/configuration/sentry/

ewaterman · 2024-11-05T21:30:35Z

src/main/resources/sentry.properties

+
+# If you ever need to debug Sentry changes locally, you can set the following properties. Note that any values
+# defined here will be overridden by "SENTRY_DSN" and "SENTRY_ENVIRONMENT" environment variables if those are defined.
+dsn=${SENTRY_DSN_BACKEND}


See https://github.com/openboxes/openboxes-devops/pull/74 for details on how this env var is being set.

jmiranda · 2024-11-12T20:33:08Z

grails-app/conf/logback.xml

            <pattern>
-                %date{ISO8601} %5level [%-10.10thread] %-40.40logger{40}: %message%n%xException
+                <!-- https://logback.qos.ch/manual/layouts.html -->
+                %cyan(%date{ISO8601}) %highlight(%-5level) %magenta([%thread]) %yellow(%logger{40}): %message%n%xException


I might have been wrong about the color issue. Here's the PR I was referring a few weeks ago re: "Matt removed something fancy that was causing issues on Bamboo". Turns out it was due to webpack, not our logging configuration.

#3052

jmiranda · 2024-11-12T20:34:47Z

grails-app/conf/logback.xml

    <logger name="org.hibernate.transform" level="info" />
    <logger name="org.hibernate.tool" level="info" />
-    <logger name="org.hibernate.type" level="info" />
+    <logger name="org.hibernate.type" level="info" /><!-- https://www.javacodegeeks.com/2012/10/logging-hibernate-sql.html -->


Would you mind adding a comment to explain the blog post? Are you including to explain why it's used i.e. that it's to improve SQL logging?

Yeah exactly. I'll add a comment to clarify

jmiranda · 2024-11-12T20:40:39Z

src/main/groovy/org/pih/warehouse/monitoring/GrailsHttpTransactionNameProvider.groovy

+        // If we wanted our logs to look more like HTTP Requests, we could have done the following instead, but
+        // we decided that "invoiceApi/list" is easier to debug than "GET /openboxes/api/invoices".
+        //return "${var1.method} ${var1.requestURI}"
+        return "${var1.getAttribute("org.grails.CONTROLLER_NAME_ATTRIBUTE")}/${var1.getAttribute("org.grails.ACTION_NAME_ATTRIBUTE")}"


I understand that invoiceApi/list is easier to parse and figure out where to look, but using the URL Mapping seems to be a better choice.

Is there a way to make the choice configurable? I assume the easiest way would be to leave this class alone and handle the configuration in the SentryGrailsTracingFilter.

Note: I am not asking you to do it, just wondering what it would entail.

I like this approach better (implementing a Sentry transaction name provider) but just wanted to point out that we're setting the transaction name for New Relic here.

https://github.com/openboxes/openboxes/blob/d0a8cab0ba9a4333529c05e39aac3515ddaffe39/grails-app/conf/openboxes/NewRelicFilters.groovy

Perhaps someday we'll want the transaction names to be consistent.

Yeah we could make it configurable via an app setting. I already have the code for both options so I have no problem adding a property for this.

jmiranda · 2024-11-12T20:42:10Z

src/main/groovy/org/pih/warehouse/monitoring/GrailsHttpTransactionNameProvider.groovy

+ */
+class GrailsHttpTransactionNameProvider implements TransactionNameProvider {
+
+    String provideTransactionName(HttpServletRequest var1) {


Could you change the var1 to request? Uness there's something I'm missing the var1 had me double-take a few times during the review of this code.

I used var1 because that's what the abstract method in TransactionNameProvider uses and I remember you previously asked me to maintain method arg names from the interface. I agree that request is clearer though so I can change it

Ugh. Yes, I did. :)

I assumed the arg1 was the default arg name provided by Intellij, but I guess that makes sense.

jmiranda · 2024-11-12T20:43:40Z

src/main/groovy/org/pih/warehouse/monitoring/SentryGrailsTracingFilter.groovy

+    }
+
+    SentryGrailsTracingFilter(final IHub hub) {
+        super(hub, new GrailsHttpTransactionNameProvider())


I assume here we could use SpringMvcTransactionNameProvider to get the default behavior. Does the default behavior use the URL mapping?

yes but it doesn't work with grails (nothing will ever get logged to sentry) so we'd need to provide our own implementation regardless. See that forum post linked in the class docstring if you're curious

jmiranda · 2024-11-12T20:46:19Z

src/main/groovy/org/pih/warehouse/monitoring/GrailsHttpTransactionNameProvider.groovy

+
+    TransactionNameSource provideTransactionSource() {
+        // Do what SpringMvcTransactionNameProvider does, which is the provider that Sentry normally uses.
+        return TransactionNameSource.ROUTE


I don't know if this really matters, but is this accurate? I feel like custom would be more accurate since it doesn't seem like we're dealing with the parameters at all.

https://javadoc.io/static/io.sentry/sentry/6.31.0/io/sentry/protocol/TransactionNameSource.html

Yeah good point. I originally had it using the URI approach, so ROUTE made more sense, but since we switched I think maybe COMPONENT makes the most sense since it's based off a Grails controller and method?

jmiranda · 2024-11-12T21:04:30Z

src/main/resources/spy.properties

+# (mainly the P6LogFactory) so that we don't flood our system with P6Spy log files that we don't use.
+# https://blog.sentry.io/troubleshooting-spring-boot-applications-with-sentry/#jdbc-instrumentation
+modulelist=com.p6spy.engine.spy.P6SpyFactory
+driverlist=com.mysql.jdbc.Driver


Do we still need this if p6spy is configured on the server?

Enabling p6spy via:

dataSource: driverClassName: com.p6spy.engine.spy.P6SpyDriver url: jdbc:p6spy:mysql://localhost:3306/openboxes?serverTimezone=UTC&useSSL=false

still requires the settings here for p6spy to be able to know what "real" driver to forward to after it does its thing.

from what I saw when implementing this, everyone was configuring this via spy.properties. I can't remember if it was a requirement for it to work or if it was smart enough to look in the application.yml file. I can test it out again.

Yeah that's what i was thinking. Not a big deal. I just didn't want to keep a file around that wasn't necessary.

jmiranda

Review my comments in case I've uncovered anything that needs to be addressed. Otherise it looks good to me.

codecov · 2024-11-13T21:31:30Z

Codecov Report

Attention: Patch coverage is 16.21622% with 31 lines in your changes missing coverage. Please review.

Please upload report for BASE (develop@43489be). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...onitoring/GrailsHttpTransactionNameProvider.groovy	4.76%	20 Missing ⚠️
...onitoring/SentryServletContainerInitializer.groovy	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             develop   #4898   +/-   ##
=========================================
  Coverage           ?   7.62%           
  Complexity         ?     811           
=========================================
  Files              ?     598           
  Lines              ?   42190           
  Branches           ?   10261           
=========================================
  Hits               ?    3217           
  Misses             ?   38504           
  Partials           ?     469

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

add support for Sentry performance tracing

e6e943a

ewaterman requested review from awalkowiak and jmiranda October 15, 2024 21:17

ewaterman self-assigned this Oct 15, 2024

github-actions bot added type: maintenance Code improvements, optimizations and refactors, dependency upgrades... domain: backend Changes or discussions relating to the backend server flag: config change Hilights a pull request that contains a change to the app config labels Oct 15, 2024

move Sentry properties to sentry.properties file and document each fi…

66612ea

…le better

ewaterman added 2 commits October 25, 2024 10:08

update sentry docstrings for clarity

073f3d6

Log database queries to Sentry via P6Spy

c721f21

ewaterman commented Oct 29, 2024

View reviewed changes

ewaterman commented Oct 30, 2024

View reviewed changes

ewaterman added 2 commits October 30, 2024 13:09

simplify how we set transaction name for performance traces

336afd2

disable p6spy by default

1013c42

use new SENTRY_DSN_BACKEND env var unless SENTRY_DSN exists

10b830a

ewaterman commented Nov 5, 2024

View reviewed changes

ewaterman assigned awalkowiak Nov 7, 2024

jmiranda reviewed Nov 12, 2024

View reviewed changes

jmiranda approved these changes Nov 12, 2024

View reviewed changes

add comments and server setting for switching transaction name format

c87bcc8

ewaterman merged commit edfb53f into develop Nov 14, 2024

ewaterman deleted the maintenance/OBPIH-6733-sentry-performance branch November 14, 2024 18:46

ewaterman mentioned this pull request Nov 14, 2024

OBPIH-6732 Add Sentry performance and error tracing to React #4943

Merged

Uh oh!

Conversation

ewaterman commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Description of Change

Uh oh!

ewaterman commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman commented Oct 31, 2024

Uh oh!

jmiranda commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ewaterman Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmiranda left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 13, 2024

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ewaterman commented Oct 15, 2024 •

edited

Loading

ewaterman commented Oct 18, 2024 •

edited

Loading

ewaterman Nov 12, 2024 •

edited

Loading