Observability provider #154

fmeheust · 2025-02-25T14:23:48Z

This provider is intended to replace the Open Telemetry provider and integrates with both Open Telemetry and Java Flight Recorder.

ojdbc-provider-observability/README.md

ejannett · 2025-03-05T07:56:57Z

...rc/main/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListenerProvider.java

+  /**
+   * Logger
+   */
+  private Logger logger = Logger.getLogger(ObservabilityTraceEventListenerProvider.class.getName());


not static ?

ejannett · 2025-03-05T07:58:00Z

...rc/main/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListenerProvider.java

+  protected static final MBeanServer server = ManagementFactory.getPlatformMBeanServer();;
+
+
+  static {


What is the need of this static block ?

ejannett · 2025-03-05T07:59:54Z

...rc/main/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListenerProvider.java

+    try {
+      objectName = new ObjectName(MBEAN_OBJECT_NAME);
+    } catch (MalformedObjectNameException e) {
+      objectName = null;


Our ObjectName name is fixed. This exception case will never happen.
And what will be the later consequences of objectName being null ?

ejannett · 2025-03-05T08:01:25Z

...rc/main/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListenerProvider.java

+
+  @Override
+  public TraceEventListener getTraceEventListener(Map<Parameter, CharSequence> map) {
+    try {


It seems quite costly here, especially as we are registering a singleton. Why not moving this elsewhere ?

ejannett · 2025-03-05T10:43:09Z

...-observability/src/main/java/oracle/jdbc/provider/observability/tracers/otel/OTelTracer.java

+
+  private String initAndGetTraceState(Span span) {
+    final TraceState traceState = span.getSpanContext().getTraceState();
+    final StringBuilder stringBuilder = new StringBuilder();


Why not

final StringBuilder stringBuilder = new StringBuilder("tracestate: "); traceState.forEach((k, v) -> stringBuilder.append(k).append("=").append(v)); return stringBuilder.append("\r\n").toString();

Are we not missing a comma or something else between key/values ?

ejannett · 2025-03-05T10:46:18Z

...-observability/src/main/java/oracle/jdbc/provider/observability/tracers/otel/OTelTracer.java

+    final String parentId = spanContext.getSpanId();
+    final String traceFlags = spanContext.getTraceFlags().toString();
+
+    return String.format("traceparent: %s-%s-%s-%s\r\n",


What is the "contract" regarding the format return ?
It looks like kind of toString() but this is used in the API which usually imply stronger specification.
(same apply to return of initAndGetTraceState())

ejannett · 2025-03-05T10:51:22Z

...rc/main/java/oracle/jdbc/provider/opentelemetry/OpenTelemetryTraceEventListenerProvider.java

+  /**
+   * Logger
+   */
+  private Logger logger = Logger.getLogger(OpenTelemetryTraceEventListenerProvider.class.getName());


Should be static.

General note about loggers : unless there is a justification to have a specific logger for a class, We usually see loggers being instantiated in a higher level context
i.e logger = Logger.getLogger(OpenTelemetryTraceEventListenerProvider.class..getPackage().getName());

ejannett · 2025-03-05T10:52:08Z

...rc/main/java/oracle/jdbc/provider/opentelemetry/OpenTelemetryTraceEventListenerProvider.java

+   */
+  private Logger logger = Logger.getLogger(OpenTelemetryTraceEventListenerProvider.class.getName());
+
+  private static ObjectName objectName;


same remark as for the use of ObjectName above

ejannett · 2025-03-05T10:53:45Z

...ty/src/test/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListenerTest.java

+          assertEquals("OK", resultSet.getString(1));
+        }
+      }
+      recording.stop();


General remark about test classes: please few comments about what they do when appropriate

MouhsinElmajdouby · 2025-02-28T14:30:47Z

ojdbc-provider-observability/README.md

+driver which will be notified whenever events are generated in the driver and 
+will publish these events into Open Telemetry. These events include:
+ * roundtrips to the database server
+ * AC begin and sucess


Typo: 'sucess' should be corrected to 'success'.

MouhsinElmajdouby · 2025-02-28T14:32:52Z

ojdbc-provider-observability/README.md

+ * AC begin and sucess
+ * VIP down event
+
+The following attributes are added the the traces for each event:


Typo: 'the the' should be corrected to 'to the'.

MouhsinElmajdouby · 2025-02-28T14:37:18Z

...bility/src/main/java/oracle/jdbc/provider/observability/ObservabilityTraceEventListener.java

@@ -0,0 +1,90 @@
+package oracle.jdbc.provider.observability;


missing copyright

MouhsinElmajdouby · 2025-02-28T14:40:32Z

...c/main/java/oracle/jdbc/provider/observability/configuration/ObservabilityConfiguration.java

+   */
+  private static final String SENSITIVE_DATA_ENABLED = "oracle.jdbc.provider.observability.sensitiveDataEnabled";
+
+  private static final ReentrantLock observabilityConfiguraitonLock = new ReentrantLock();


Typo in variable name: 'observabilityConfiguraitonLock' should be 'observabilityConfigurationLock'.

MouhsinElmajdouby · 2025-02-28T14:52:57Z

...c/main/java/oracle/jdbc/provider/observability/configuration/ObservabilityConfiguration.java

+    String enabledTracers = System.getProperty(ENABLED_TRACERS, "OTEL,JFR");
+    String sensitiveDataEnabled = System.getProperty(SENSITIVE_DATA_ENABLED, "false");
+    setEnabledTracers(enabledTracers);
+    setSensitiveDataEnabled(Boolean.valueOf(sensitiveDataEnabled));


Consider using Boolean.parseBoolean() instead of Boolean.valueOf() as it directly returns a primitive boolean rather than creating a Boolean object that needs to be unboxed.

MouhsinElmajdouby · 2025-03-04T16:20:48Z

...vider-observability/src/main/java/oracle/jdbc/provider/observability/tracers/TracerType.java

+   */
+  JFR(new JFRTracer());
+
+  private ObservabilityTracer tracer;


we can mark tracer as final

MouhsinElmajdouby · 2025-03-04T16:44:28Z

...ervability/src/main/java/oracle/jdbc/provider/observability/tracers/ObservabilityTracer.java

+   * @return a user context object that is passed to the next call on this 
+   * Connection. May be null.
+   */
+  Object traceRoundtrip(Sequence sequence, TraceContext traceContext, Object userContext);


maybe we need to rename this method to traceRoundTrip for consistency

MouhsinElmajdouby · 2025-03-04T16:52:40Z

...rc/main/java/oracle/jdbc/provider/opentelemetry/OpenTelemetryTraceEventListenerProvider.java

+  /**
+   * Name of the property used to enable or disable this listener.
+   */
+  public static final String OPEN_TELEMENTRY_TRACE_EVENT_LISTENER_ENABLED = "oracle.jdbc.provider.opentelemetry.enabled";


There's a typo in the constant name OPEN_TELEMENTRY. It should be OPEN_TELEMETRY

MouhsinElmajdouby · 2025-03-04T16:52:54Z

...rc/main/java/oracle/jdbc/provider/opentelemetry/OpenTelemetryTraceEventListenerProvider.java

+   * Name of the property used to enable or disable sensitive data for this
+   * listener.
+   */
+  public static final String OPEN_TELEMENTRY_TRACE_EVENT_LISTENER_SENSITIVE_ENABLED = "oracle.jdbc.provider.opentelemetry.sensitive-enabled";


MouhsinElmajdouby · 2025-03-04T16:55:15Z

...rc/main/java/oracle/jdbc/provider/opentelemetry/OpenTelemetryTraceEventListenerProvider.java

+  public TraceEventListener getTraceEventListener(Map<Parameter, CharSequence> map) {
+    try {
+      if (!server.isRegistered(objectName)) {
+        boolean enabled = Boolean.valueOf(System.getProperty(OPEN_TELEMENTRY_TRACE_EVENT_LISTENER_ENABLED, "true"));


same here Boolean.parseBoolean() instead of Boolean.valueOf()

… listeners

ejannett

looks good.

MouhsinElmajdouby · 2025-03-10T09:34:42Z

ojdbc-provider-observability/example-test.properties

@@ -0,0 +1,86 @@
+################################################################################
+# Copyright (c) 2024 Oracle and/or its affiliates.


we should update the copyright to 2025

…dbc-extensions into observability-provider

fmeheust added 19 commits January 23, 2025 11:41

First commit

6db2af2

Added static Events and tests

cf6b82a

Replaced set by list

5f5fdb8

Changes to tests

2546b01

Added test name

50211a2

Check connectionID

535737c

Static JFR events

65753e8

Merge remote-tracking branch 'origin/main' into observability-provider

fe09409

Merged from main

ce3601d

Documentation and tests

bc11b39

Added execution events to JFR

b469a80

Documentation changes

a6cd874

Fixed tests

8142aa0

Observability configuration test

01cfd12

Added debug info

ad7df96

Run tests one by one

ac08356

Test Order

8478b83

Added comment for test run configuration.

6b03e6f

Backward compatibility

c8a8cdf

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 25, 2025

fmeheust requested a review from MouhsinElmajdouby February 25, 2025 17:31

Locks and event labels

dee07b4

fmeheust requested a review from ejannett March 3, 2025 14:44

ejannett reviewed Mar 5, 2025

View reviewed changes

MouhsinElmajdouby reviewed Mar 5, 2025

View reviewed changes

fmeheust added 5 commits March 7, 2025 09:29

Refactoring and added possibility to have different configuration and…

c649e57

… listeners

Changed action version for deprecated action

65c7765

Set version in pom files correctly

f6e972f

Fixed tests when running in the same JVM

8f7b15d

Unique identifier and refactoring.

1157aa3

Merge branch 'main' into observability-provider

ed0c89e

ejannett approved these changes Mar 10, 2025

View reviewed changes

MouhsinElmajdouby reviewed Mar 10, 2025

View reviewed changes

fmeheust added 2 commits March 10, 2025 11:48

Corrected copyright

dd9de05

Merge branch 'observability-provider' of https://github.com/oracle/oj…

59f3262

…dbc-extensions into observability-provider

MouhsinElmajdouby approved these changes Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Observability provider #154

Observability provider #154

fmeheust commented Feb 25, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

ejannett Mar 5, 2025

MouhsinElmajdouby Feb 28, 2025

MouhsinElmajdouby Feb 28, 2025

MouhsinElmajdouby Feb 28, 2025

MouhsinElmajdouby Feb 28, 2025

MouhsinElmajdouby Feb 28, 2025

MouhsinElmajdouby Mar 4, 2025

MouhsinElmajdouby Mar 4, 2025

MouhsinElmajdouby Mar 4, 2025

MouhsinElmajdouby Mar 4, 2025

MouhsinElmajdouby Mar 4, 2025

ejannett left a comment

MouhsinElmajdouby Mar 10, 2025

		protected static final MBeanServer server = ManagementFactory.getPlatformMBeanServer();;


		static {

		@@ -0,0 +1,86 @@
		################################################################################
		# Copyright (c) 2024 Oracle and/or its affiliates.

Observability provider #154

Are you sure you want to change the base?

Observability provider #154

Conversation

fmeheust commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejannett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment