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.

Sign up for GitHub

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

Jump to bottom

Broadened catch clause when extracting es query body #2993

Merged
merged 7 commits into from
Feb 6, 2023
Merged

Broadened catch clause when extracting es query body #2993

merged 7 commits into from
Feb 6, 2023

Conversation

JonasKunz
Copy link
Contributor

What does this PR do?

A user reported that our instrumentation of the elasticsearch client triggers an error when using hiberantesearch:

java.lang.UnsupportedOperationException: Not implemented! Expected to produce content only over produceContent(), or writeTo(OutputStream) if blocking calls are acceptable for your use case.
at org.hibernate.search.elasticsearch.util.impl.GsonHttpEntity.getContent(GsonHttpEntity.java:164)
at co.elastic.apm.agent.esrestclient.ElasticsearchRestClientInstrumentationHelper.createClientSpan(ElasticsearchRestClientInstrumentationHelper.java:105)
at co.elastic.apm.agent.esrestclient.v6_4.ElasticsearchClientAsyncInstrumentation$ElasticsearchRestClientAsyncAdvice.onBeforeExecute(ElasticsearchClientAsyncInstrumentation.java:68)

This error is not caught by our current instrumentation. With this PR, we simply make our code more defensive to only fail the recording of the query and not the entire adivce.

I would suggest not implementing any special case handling for hibernatesearch, as the issue has been fixed there since version 6:
hibernate/hibernate-search@8d603b1

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else

@apmmachine
Copy link
Contributor

apmmachine commented Jan 30, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview previewSnapshots

Expand to view the summary

Build stats

  • Start Time: 2023-02-06T09:14:57.851+0000

  • Duration: 55 min 24 sec

Test stats 🧪

Test Results
Failed 0
Passed 3483
Skipped 43
Total 3526

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark tests.

  • run jdk compatibility tests : Run the JDK Compatibility tests.

  • run integration tests : Run the Agent Integration tests.

  • run end-to-end tests : Run the APM-ITs.

  • run windows tests : Build & tests on windows.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@JonasKunz JonasKunz added the ci:agent-integration Enables agent integration tests in build pipeline label Feb 1, 2023
JonasKunz
JonasKunz commented Feb 1, 2023
View reviewed changes
apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java
return new AgentBuilder.Transformer.ForAdvice(withCustomMapping)
.advice(instrumentationStats.shouldMeasureMatching() ? statsCollectingMatcher : matcher, instrumentation.getAdviceClassName())
.include(ClassLoader.getSystemClassLoader(), PrivilegedActionUtils.getClassLoader(instrumentation.getClass()))
.withExceptionHandler(PRINTING);
.withExceptionHandler(new Advice.ExceptionHandler.Simple(exceptionHandler));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[For Reviewer]
I noticed that when an exception is thrown by an advice and suppressed by bytebuddy, it is only printed to stdout and not to the agent logs. I don't know if this already bit us in the past but I think it might in the future: If an advice throws an exception and we only have the agentlogs without the application stdout, we might not see the actual root cause: E.g. we might see messages due to orphaned spans without the actual exception causing this.

I fixed this in this PR by adjusting how exceptions suppressed by bytebuddy are logged. Feel free to object if you don't like it, I have no problem with removing it from this PR.

You can verify the behaviour via the existing InstrumentationTest.testSuppressException test.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! We were scratching our heads how to get advice exceptions into the agent logs without manually writing byte code for the stack manipulation. 👏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that's nice to hear! I initially tried a different approach (wrapping the MethodHandles, see 326db67#diff-2ec63f545539ff7d65f52ef01087a18c7cdd9eb7130de156fb412ae6fe9e580eR465), but then noticed that some Advices might intentionally throw exceptions, at least according to the tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!! 👏

@JonasKunz JonasKunz marked this pull request as ready for review February 1, 2023 15:16
@JonasKunz JonasKunz requested a review from eyalkoren February 1, 2023 15:17
@github-actions
Copy link

github-actions bot commented Feb 1, 2023

/test

eyalkoren
eyalkoren reviewed Feb 5, 2023
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Beautiful addition, well done!
Two minor comments.

apm-agent-bootstrap/src/main/java/bootstrap/dispatcher/IndyBootstrapDispatcher.java Outdated Show resolved Hide resolved
apm-agent-core/src/main/java/co/elastic/apm/agent/bci/ElasticApmAgent.java
return new AgentBuilder.Transformer.ForAdvice(withCustomMapping)
.advice(instrumentationStats.shouldMeasureMatching() ? statsCollectingMatcher : matcher, instrumentation.getAdviceClassName())
.include(ClassLoader.getSystemClassLoader(), PrivilegedActionUtils.getClassLoader(instrumentation.getClass()))
.withExceptionHandler(PRINTING);
.withExceptionHandler(new Advice.ExceptionHandler.Simple(exceptionHandler));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!! 👏

apm-agent-core/src/main/java/co/elastic/apm/agent/bci/IndyBootstrap.java Outdated Show resolved Hide resolved
@JonasKunz JonasKunz requested a review from eyalkoren February 6, 2023 09:20
@JonasKunz JonasKunz merged commit dfe033d into elastic:main Feb 6, 2023
@JonasKunz JonasKunz deleted the es-querybody-error branch February 6, 2023 11:00
@SylvainJuge SylvainJuge mentioned this pull request Apr 12, 2023
Closed
19 tasks
@JonasKunz JonasKunz mentioned this pull request Jun 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixbarny felixbarny felixbarny left review comments

@eyalkoren eyalkoren eyalkoren approved these changes

Assignees
No one assigned
Labels
agent-java ci:agent-integration Enables agent integration tests in build pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JonasKunz @apmmachine @felixbarny @eyalkoren