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

Instrument spring-batch #1843

Merged
merged 8 commits into from
Dec 16, 2020
Merged

Instrument spring-batch #1843

merged 8 commits into from
Dec 16, 2020

Conversation

mateuszrzeszutek
Copy link
Member

Related to #1842

This PR introduces just a "skeleton" instrumentation that does not log any span attributes, just creates the spans. Span names may change once a spec PR emerges.

@jkwatson
Copy link
Contributor

jkwatson commented Dec 7, 2020

My personal opinion: We should do this in much smaller chunks. Keep PRs to a few hundred lines. It's simply not possible (for me) to do an effective review of 1300+ lines of code.

trask
trask reviewed Dec 8, 2020
View reviewed changes
Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Does it simplify things to instrument Job.execute() and Step.execute() directly instead of the registration and use of execution listeners? I'm not sure what the equivalent for Chunk would be..

...java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java Outdated Show resolved Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java Outdated Show resolved Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java Outdated Show resolved Hide resolved
instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/config/Config.java Show resolved Hide resolved
...java/io/opentelemetry/javaagent/instrumentation/spring/batch/chunk/ChunkExecutionTracer.java Outdated Show resolved Hide resolved
...ain/java/io/opentelemetry/javaagent/instrumentation/spring/batch/job/JobExecutionTracer.java Outdated Show resolved Hide resolved
...n/java/io/opentelemetry/javaagent/instrumentation/spring/batch/step/StepExecutionTracer.java Outdated Show resolved Hide resolved
...o/opentelemetry/javaagent/instrumentation/spring/batch/SpringBatchInstrumentationModule.java Outdated Show resolved Hide resolved
anuraaga
anuraaga reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

It might help with @jkwatson's suggestion of splitting into steps (pun intended! :-) Would it help to start from library instrumentation? It reduces load to only be looking at Spring APIs first, and separately reviewing auto-instrumentation tricks.

@mateuszrzeszutek
Copy link
Member Author

Would it help to start from library instrumentation?

That was my first approach actually 😅 But unfortunately I need to use InstrumentationContext in all listeners - Spring Batch's built in ExecutionContext is periodically stored in the DB, and trying to keep our context & scope there could be too intrusive (and there's no separate execution context for chunk).

It might help with @jkwatson's suggestion of splitting into steps

👍
I'll try splitting this PR into 2-3 smaller ones that will be easier to review

Does it simplify things to instrument Job.execute() and Step.execute() directly instead of the registration and use of execution listeners? I'm not sure what the equivalent for Chunk would be..

There's no equivalent for Chunk unfortunately. For Job and Step I could instrument execute() directly and I believe that it would be perfectly fine, but I wanted to rely on the tools that the framework provides - we can be sure that listener.after(Step|Job) will be called after a particular unit of work no matter what kind of async/concurrent processing is being used.

@mateuszrzeszutek mateuszrzeszutek force-pushed the instrument-spring-batch branch 2 times, most recently from ea534ad to 5ec3e87 Compare December 8, 2020 12:01
@mateuszrzeszutek mateuszrzeszutek mentioned this pull request Dec 8, 2020
Merged
@mateuszrzeszutek mateuszrzeszutek mentioned this pull request Dec 8, 2020
Merged
@mateuszrzeszutek
Copy link
Member Author

@anuraaga @jkwatson @iNikem @trask Please review this PR if you find a bit of free time 🙏
I've slimmed it down a bit, hope it's easier to read now. Thanks!

Mateusz Rzeszutek and others added 3 commits December 16, 2020 11:06
@iNikem iNikem merged commit f82e910 into open-telemetry:master Dec 16, 2020
bhautikpip pushed a commit to bhautikpip/opentelemetry-java-instrumentation that referenced this pull request Dec 18, 2020
@trask @bhautikpip
Instrument spring-batch (open-telemetry#1843)
f3008fd 
* Instrument spring-batch: job & step

* Instrument spring-batch: job & step - code review comments

* Instrument spring-batch: job & step - code review comments

* Instrument spring-batch: job & step - code review comments

* spotless

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* Instrument spring-batch: job & step - code review comments

* spotless

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@mateuszrzeszutek mateuszrzeszutek deleted the instrument-spring-batch branch February 5, 2021 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iNikem iNikem iNikem approved these changes

@anuraaga anuraaga anuraaga approved these changes

@trask trask trask approved these changes

@jkwatson jkwatson Awaiting requested review from jkwatson

@tylerbenson tylerbenson Awaiting requested review from tylerbenson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mateuszrzeszutek @jkwatson @anuraaga @trask @iNikem