Addressing review comments

oracle · Dec 6, 2024 · 6d41a4a · 6d41a4a
1 parent 1dcfe1f
commit 6d41a4a
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 114 deletions.
diff --git 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/...provider-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListener.java b/...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/...-jfr/src/main/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerProvider.java b/...-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:
- * <ul>
- * <li>
- *  <b>enable</b>: if true, the trace event listener will create events for 
- *  each round trip otherwise no events will be created.
- * </li>
- * <li>
- *  <b>enableSensitiveData</b>: if true, sensitive attributes like SQL statements
- *  and database used will be added to the event, otherwise sensitive attributes
- *  will not be added.
- * </li>
- * </ul>
- */
+ * 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<? extends Parameter> getParameters() {
 
-    List<Parameter> parameters = Arrays.asList(ENABLE_SENSITIVE_DATA, ENABLE);
+    List<Parameter> parameters = Collections.singletonList(ENABLE_SENSITIVE_DATA);
     return parameters;
   }
 
   @Override
   public TraceEventListener getTraceEventListener(Map<Parameter, CharSequence> 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<Parameter, CharSequence> 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
@@ -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 {
    *  <li>User</li>
    * </ul>
    */
-  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<AnnotationElement> eventAnnotations = new ArrayList<AnnotationElement>();
     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<ValueDescriptor> fields = new ArrayList<ValueDescriptor>();

diff --git a/...ider-jfr/src/test/java/oracle/jdbc/provider/jfr/FlightRecorderTraceEventListenerTest.java b/...ider-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,62 +64,31 @@ 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
   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));
-  }
-
-
 }