-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Oracle JDBC Java Flight Recorder Provider #133
base: main
Are you sure you want to change the base?
Conversation
ojdbc-provider-jfr/README.md
Outdated
application's classpath and set the following connection property : | ||
|
||
```java | ||
oracle.jdbc.provider.traceEventListener=java-flight-recorder-trace-event-listener-providerr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo : double 'r'
ojdbc-provider-jfr/pom.xml
Outdated
<dependencies> | ||
<dependency> | ||
<groupId>com.oracle.database.jdbc</groupId> | ||
<artifactId>ojdbc8</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 8 if you compile in 11 ?
* <ul> | ||
* Two parameters can be used to configure the trace event listener: | ||
* <li> | ||
* <b>enable</b>: if true, the trace event listener will create events for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more details about why/when/how we can switch this
on and off ?
If this user visilble, may a note in the README can help also
nit-picking: Is this is not already covered by design add/removing the property? (i.e pluging in and out the provider?). What the use-case of this flag ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, you can assume that if you add the jar + the property that you want it to be enabled, and JFR events will only be created when JFR recording is running. We can argue that the property is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. That's not a big deal but it may be confusing at the end.
especially if other extension do not work the same.
You may even argue that this may be a feature at provider framework level.
@Override | ||
public Object roundTrip(Sequence sequence, TraceContext traceContext, Object userContext) { | ||
if (!enabled) { | ||
logger.log(Level.FINE, "FlightRecorderTraceEventListener is disabled"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FINEST can be enough.
But anyway, That will fire at each roundtrip. are you sure you will never flood the logs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them while I was testing, but there is no need to log here.
event.begin(); | ||
return event; | ||
} else { | ||
logger.log(Level.FINE, "Received after event"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a little bit of info about the event, otherwise the value is limited
no ?
(we assume that roundtrip listeners work, right ? :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them while I was testing, but there is no need to log here.
event.set(6, traceContext.user()); | ||
} | ||
event.end(); | ||
event.commit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
being curious : is that can raise an exception ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only method that could raise an exception is Event.set, that's if the indexes are not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough then
* <li>User: create if sensitive is enabled</li> | ||
* </ul> | ||
*/ | ||
public static Event createEvent(String databaseOperation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not use the enum we have for operation now ?
return f.newEvent(); | ||
} | ||
|
||
private static String toCamelCase(String value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
javadoc here?
** OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
** SOFTWARE. | ||
*/ | ||
package oracle.jdbc.provider.jfr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few wordings around test may help maintainers to understand
expectation etc...
|
||
@BeforeAll | ||
public static void createStaticMock() { | ||
eventFactory = Mockito.mockStatic(OracleEventFactory.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A general question for the entire project:
How can I test against a real DB ? if not would be interesting ?
.github/workflows/run-tests.yaml
Outdated
@@ -16,7 +16,7 @@ jobs: | |||
- name: Set up JDK 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Set up JDK 11" , no ?
ojdbc-provider-jfr/README.md
Outdated
# Oracle JDBC Java Flight Recorder Provider | ||
|
||
This module contains a provider for integration between Oracle JDBC and | ||
Java Flight Record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Recorder" and mention Mission Control to enable and view.
ojdbc-provider-jfr/README.md
Outdated
|
||
## Installation | ||
|
||
This provider is distributed as single jar on the Maven Central Repository. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"as a single jar on the Maven Central Repository"
Trace event listener that adds round trip events to Java Flight Recorder.