Skip to content
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

Add support for MyBatis framework #10258

Merged
merged 12 commits into from
Feb 12, 2024

Conversation

steverao
Copy link
Contributor

@steverao steverao commented Jan 17, 2024

Add support for MyBatis framework, for detail can see #10186

Resolves #10186

@steverao steverao requested a review from a team as a code owner January 17, 2024 17:11
Copy link
Member

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One CI failure is that a method has to be static:

MybatisTest.java:61: warning: [MethodCanBeStatic] A private method that does not reference the enclosing instance can be static
  private void span(String spanName) {

@steverao
Copy link
Contributor Author

One CI failure is that a method has to be static:

MybatisTest.java:61: warning: [MethodCanBeStatic] A private method that does not reference the enclosing instance can be static
  private void span(String spanName) {

ok, thanks, I solved it now.

@AlchemyDing
Copy link
Member

AlchemyDing commented Jan 18, 2024

The span added this time without any additional attributes, just to show the span?

@steverao
Copy link
Contributor Author

The span added this time without any additional attributes, just to show the span?

Yes, at present, no particularly valuable attributes have been found that need to be added.

@steverao steverao requested a review from laurit January 23, 2024 06:12
@steverao
Copy link
Contributor Author

Hi @laurit @jeanbisutti, for the PR, do you have any more suggestions?

@trask
Copy link
Member

trask commented Jan 29, 2024

@laurit what do you think of making this instrumentation opt-in since it doesn't implement any semconv?

@laurit
Copy link
Contributor

laurit commented Feb 8, 2024

@steverao I create a PR (steverao#1) for your PR that should get this closer to mergeable shape. In this PR

  • instrumentation is disabled by default
  • does not rely on SqlCommand.getName() returning fullClassName.methodName (I don't know whether it is guaranteed to return that)
  • span name is based on the mapper class and method, code attributes are also generated

@steverao
Copy link
Contributor Author

steverao commented Feb 8, 2024

@steverao I create a PR (steverao#1) for your PR that should get this closer to mergeable shape. In this PR

  • instrumentation is disabled by default
  • does not rely on SqlCommand.getName() returning fullClassName.methodName (I don't know whether it is guaranteed to return that)
  • span name is based on the mapper class and method, code attributes are also generated

Thank you very much for your optimization. I think there is no problem with the other points. The instrumentation is set to disabled by default. mainly to consider that sometimes it may not be much different from the database, right?

@trask trask merged commit f777c0e into open-telemetry:main Feb 12, 2024
47 checks passed
steverao added a commit to steverao/opentelemetry-java-instrumentation that referenced this pull request Feb 16, 2024
Co-authored-by: Lauri Tulmin <tulmin@gmail.com>
@steverao steverao deleted the steverao/main-mybatis branch February 26, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for MyBatis framework
5 participants