From 6d41a4ad9fa9c9e04cdfddcde9d3e75f8851036e Mon Sep 17 00:00:00 2001 From: Fernanda Meheust Date: Fri, 6 Dec 2024 17:30:15 +0100 Subject: [PATCH] Addressing review comments --- ojdbc-provider-jfr/README.md | 4 +- .../jfr/FlightRecorderTraceEventListener.java | 16 +--- ...ghtRecorderTraceEventListenerProvider.java | 36 +++------ .../jdbc/provider/jfr/OracleEventFactory.java | 7 +- .../FlightRecorderTraceEventListenerTest.java | 79 +++---------------- 5 files changed, 28 insertions(+), 114 deletions(-) diff --git a/ojdbc-provider-jfr/README.md b/ojdbc-provider-jfr/README.md index 924995c8..ecf4a023 100644 --- a/ojdbc-provider-jfr/README.md +++ b/ojdbc-provider-jfr/README.md @@ -31,9 +31,9 @@ The coordinates for the latest release are: ## Usage -To use the Oracle JDBC provider for Open Telemetry just add the artifact to the +To use the Oracle JDBC provider for Java Flight Recorder just add the artifact to the application's classpath and set the following connection property : ```java -oracle.jdbc.provider.traceEventListener=java-flight-recorder-trace-event-listener-providerr +oracle.jdbc.provider.traceEventListener=java-flight-recorder-trace-event-listener-provider ``` diff --git a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListener.java b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListener.java index de876d2c..804fbe5c 100644 --- a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListener.java +++ b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListener.java @@ -50,21 +50,18 @@ */ public class FlightRecorderTraceEventListener implements TraceEventListener { - private final boolean enabled; private final boolean enableSensitiveData; private static Logger logger = Logger.getLogger(FlightRecorderTraceEventListener.class.getName()); /** * Constructor - * @param enable if false, no events are created. * @param enableSensitiveData if true, events will contain sensitive information like * SQL statements and usernames. */ - public FlightRecorderTraceEventListener(boolean enable, boolean enableSensitiveData) { - this.enabled = enable; + public FlightRecorderTraceEventListener(boolean enableSensitiveData) { this.enableSensitiveData = enableSensitiveData; - logger.log(Level.INFO, "FlightRecorderTraceEventListener started enabled " + this.enabled + " sensitive data enabled " + this.enableSensitiveData); + logger.log(Level.INFO, "FlightRecorderTraceEventListener started: sensitive data enabled " + this.enableSensitiveData); } /** @@ -73,20 +70,13 @@ public FlightRecorderTraceEventListener(boolean enable, boolean enableSensitiveD */ @Override public Object roundTrip(Sequence sequence, TraceContext traceContext, Object userContext) { - if (!enabled) { - logger.log(Level.FINE, "FlightRecorderTraceEventListener is disabled"); - return null; - } if (sequence.equals(Sequence.BEFORE)) { - logger.log(Level.FINE, "Received before event"); Event event = OracleEventFactory.createEvent( - traceContext.databaseOperation()); + traceContext.databaseFunction()); event.begin(); return event; } else { - logger.log(Level.FINE, "Received after event"); if (userContext != null) { - logger.log(Level.FINE, "Received after event not empty"); Event event = (Event) userContext; event.set(0, traceContext.getConnectionId()); event.set(1, traceContext.databaseOperation()); diff --git a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerProvider.java b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerProvider.java index f5797447..f3e3ae49 100644 --- a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerProvider.java +++ b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerProvider.java @@ -40,6 +40,7 @@ import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -48,19 +49,12 @@ /** * Implements a TraceEventListenerProvider for FlightRecorderTraceEventListener. - * Two parameters can be used to configure the trace event listener: - * - */ + * By default sensitive attributes like SQL statements and database used will + * be added to the event, the configureation property "enableSensitiveData" can + * be used to enable sensitive attributes to be added to the event. + * To enable sensitive data set the connection property + * oracle.jdbc.provider.traceEventListener.enableSensitiveData to true. +*/ public class FlightRecorderTraceEventListenerProvider implements TraceEventListenerProvider { /** @@ -80,17 +74,6 @@ public String name() { } }; - private static final Parameter ENABLE = new Parameter() { - @Override - public boolean isSensitive() { - return false; - } - @Override - public String name() { - return "enable"; - } - }; - @Override public String getName() { return TRACE_EVENT_LISTENER_NAME; @@ -99,15 +82,14 @@ public String getName() { @Override public Collection getParameters() { - List parameters = Arrays.asList(ENABLE_SENSITIVE_DATA, ENABLE); + List parameters = Collections.singletonList(ENABLE_SENSITIVE_DATA); return parameters; } @Override public TraceEventListener getTraceEventListener(Map parametersMap) { - boolean enable = getBooleanParameterValue(ENABLE, parametersMap); boolean enableSensitiveData = getBooleanParameterValue(ENABLE_SENSITIVE_DATA, parametersMap); - return new FlightRecorderTraceEventListener(enable, enableSensitiveData); + return new FlightRecorderTraceEventListener(enableSensitiveData); } private boolean getBooleanParameterValue(Parameter parameter, Map parametersMap) { diff --git a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/OracleEventFactory.java b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/OracleEventFactory.java index 0d8790e6..29888381 100644 --- a/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/OracleEventFactory.java +++ b/ojdbc-provider-jfr/src/main/java/oracle/jdbc/provider/jfr/OracleEventFactory.java @@ -47,6 +47,7 @@ import jdk.jfr.Name; import jdk.jfr.Category; import jdk.jfr.ValueDescriptor; +import oracle.jdbc.DatabaseFunction; /** * Factory class for creating Java Flight Recorder events. @@ -66,12 +67,12 @@ public class OracleEventFactory { *
  • User
    • * */ - public static Event createEvent(String databaseOperation) { - String eventName = "oracle.jdbc.provider.jfr.roundtrip." + toCamelCase(databaseOperation); + public static Event createEvent(DatabaseFunction databaseFunction) { + String eventName = "oracle.jdbc.provider.jfr.roundtrip." + databaseFunction.toString(); List eventAnnotations = new ArrayList(); eventAnnotations .add(new AnnotationElement(Name.class, eventName)); - eventAnnotations.add(new AnnotationElement(Label.class, databaseOperation)); + eventAnnotations.add(new AnnotationElement(Label.class, databaseFunction.getDescription())); eventAnnotations.add(new AnnotationElement(Category.class, new String[] { "Oracle JDBC", "Round trips" })); List fields = new ArrayList(); diff --git a/ojdbc-provider-jfr/src/test/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerTest.java b/ojdbc-provider-jfr/src/test/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerTest.java index 122db3e1..977c8c2f 100644 --- a/ojdbc-provider-jfr/src/test/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerTest.java +++ b/ojdbc-provider-jfr/src/test/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerTest.java @@ -43,6 +43,7 @@ import org.mockito.MockedStatic; import org.mockito.Mockito; import jdk.jfr.Event; +import oracle.jdbc.DatabaseFunction; import oracle.jdbc.TraceEventListener.Sequence; import oracle.jdbc.TraceEventListener.TraceContext; @@ -63,6 +64,7 @@ public static void createStaticMock() { Mockito.when(traceContext.originalSqlText()).thenReturn("Original SQL"); Mockito.when(traceContext.tenant()).thenReturn("tenant"); Mockito.when(traceContext.user()).thenReturn("user"); + Mockito.when(traceContext.databaseFunction()).thenReturn(DatabaseFunction.EXECUTE_QUERY); } @BeforeEach @@ -70,55 +72,23 @@ public void resetMocks() { eventFactory.reset(); Mockito.reset(event); eventFactory.when(() -> - OracleEventFactory.createEvent(Mockito.anyString()) + OracleEventFactory.createEvent(DatabaseFunction.EXECUTE_QUERY) ).thenReturn(event); } @Test - void testPropertiesDisabled() throws Exception { + public void roundTripSensitiveSuccessTest() throws Exception { FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(false, false); + new FlightRecorderTraceEventListener(true); traceEventListener.roundTrip(Sequence.BEFORE, traceContext, traceEventListener); // check eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(0)); - traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(0)); - } - - @Test - void testPropertiesEnabled() throws Exception { - FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(true, true); - traceEventListener.roundTrip(Sequence.BEFORE, traceContext, traceEventListener); - // check - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(1)); - - traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(1)); - - } - - @Test - public void roundTripEnabledSensitiveSuccessTest() throws Exception { - FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(true, true); - traceEventListener.roundTrip(Sequence.BEFORE, traceContext, traceEventListener); - // check - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), + () -> OracleEventFactory.createEvent(DatabaseFunction.EXECUTE_QUERY), Mockito.times(1)); traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), + () -> OracleEventFactory.createEvent(DatabaseFunction.EXECUTE_QUERY), Mockito.times(1)); Mockito.verify(event,Mockito.times(1)).set(0, traceContext.getConnectionId()); Mockito.verify(event,Mockito.times(1)).set(1, traceContext.databaseOperation()); @@ -130,26 +100,13 @@ public void roundTripEnabledSensitiveSuccessTest() throws Exception { } @Test - public void roundTripNotEnabledSensitiveSuccessTest() throws Exception { - FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(false, false); - traceEventListener.roundTrip(Sequence.BEFORE, traceContext, traceEventListener); - // check - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(0)); - traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); - - } - - @Test - public void roundTripEnabledNotSensitiveSuccessTest() throws Exception { + public void roundTripNotSensitiveSuccessTest() throws Exception { FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(true, false); + new FlightRecorderTraceEventListener(false); traceEventListener.roundTrip(Sequence.BEFORE, traceContext, null); // check eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), + () -> OracleEventFactory.createEvent(DatabaseFunction.EXECUTE_QUERY), Mockito.times(1)); traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); @@ -162,20 +119,4 @@ public void roundTripEnabledNotSensitiveSuccessTest() throws Exception { Mockito.verify(event,Mockito.times(0)).set(6, traceContext.user()); } - @Test - public void roundTripNotEnabledNotSensitiveSuccessTest() throws Exception { - FlightRecorderTraceEventListener traceEventListener = - new FlightRecorderTraceEventListener(false, false); - traceEventListener.roundTrip(Sequence.BEFORE, traceContext, traceEventListener); - // check - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(0)); - traceEventListener.roundTrip(Sequence.AFTER, traceContext, event); - eventFactory.verify( - () -> OracleEventFactory.createEvent(Mockito.anyString()), - Mockito.times(0)); - } - - }