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

Move global tracer and substitute agent-core types with API types where this is trivially possible. #3054

Merged
merged 17 commits into from
Mar 28, 2023

Conversation

raphw
Copy link
Contributor

@raphw raphw commented Mar 10, 2023

This change is mainly generated by IDEA code substitution, with minor adjustments. By using API types, the dependency on implementation is reduced and the required API surface becomes more visible. This change is in preparation of removing the core module as a dependency of the internal plugins.

@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels Mar 10, 2023
@github-actions
Copy link

👋 @raphw Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

@raphw
Copy link
Contributor Author

raphw commented Mar 10, 2023

This is mostly auto-generated. I do however wonder about one difference, and that is the change here: main...raphw:apm-agent-java:reduce-api-exposure#diff-732acc761e4310277786f5059749ac54e3f0dcfa17d8976ca3892fa7fb7d5c52L154

This particular use of TraceContext was previously a unique application of this API. Is it the same as using addLink (addSpanLink) to which I normalized this? It leads to the same asChildOf call, but also adds a link entry.

@apmmachine
Copy link
Contributor

apmmachine commented Mar 10, 2023

💚 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-03-24T23:23:53.810+0000

  • Duration: 14 min 13 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 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!)

Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Approval to allow running on CI, thorough review will follow.

CHANGELOG.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

Just a few minor comments.
In addition I think the tests are currently failing and still need some attention.

raphw and others added 16 commits March 24, 2023 23:54
…re this is trivially possible.

This change is mainly generated by IDEA code substitution, with minor adjustments. By using API types, the dependency on implementation is reduced and the required API surface becomes more visible. This change is in preparation of removing the core module as a dependency of the internal plugins.
…/main/java/co/elastic/apm/agent/kafka/SpringKafkaBatchListenerInstrumentation.java

Co-authored-by: Jonas Kunz <[email protected]>
…astic/apm/agent/grpc/GrpcHelper.java

Co-authored-by: Jonas Kunz <[email protected]>
@raphw raphw force-pushed the reduce-api-exposure branch from ee20b86 to d5c9c4d Compare March 24, 2023 22:55
@raphw
Copy link
Contributor Author

raphw commented Mar 24, 2023

Tests should be fixed by now. Mainly missing mock configurations to reflect require calls properly. And of course JmsInstrumentationIT needed an adjustment with regards to the span link change.

Copy link
Contributor

@JonasKunz JonasKunz left a comment

Choose a reason for hiding this comment

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

I think we are good to merge!
Side Note for the next PR: Would be great if you could use merge instead of force-push. This allows us to use the "Review changes since last review" feature from GH when doing review iterations.

@raphw
Copy link
Contributor Author

raphw commented Mar 27, 2023

Will do, I rebased to main, in hope that a mysterious test error would disappear by it, that's what the force push was about.

@JonasKunz
Copy link
Contributor

run elasticsearch-ci/docs

@JonasKunz JonasKunz merged commit 1fd41bc into elastic:main Mar 28, 2023
@SylvainJuge SylvainJuge mentioned this pull request Apr 12, 2023
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java community Issues and PRs created by the community triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants