-
Notifications
You must be signed in to change notification settings - Fork 67
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 Observations for send/receive #146
Conversation
.../src/main/java/org/springframework/pulsar/config/AbstractPulsarListenerContainerFactory.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Show resolved
Hide resolved
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Show resolved
Hide resolved
...src/main/java/org/springframework/pulsar/listener/DefaultPulsarMessageListenerContainer.java
Show resolved
Hide resolved
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 don't know the details of how Pulsar works, but from the point of view of Observability this looks good :) I would love to see a Zipkin screenshot to confirm the tracing part.
...pulsar/src/test/java/org/springframework/pulsar/observation/ObservationIntegrationTests.java
Show resolved
Hide resolved
b9528eb
to
42e8725
Compare
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 generated docs are missed from the commit?
spring-pulsar/src/main/java/org/springframework/pulsar/core/PulsarTemplate.java
Show resolved
Hide resolved
...src/main/java/org/springframework/pulsar/listener/DefaultPulsarMessageListenerContainer.java
Show resolved
Hide resolved
...-pulsar/src/main/java/org/springframework/pulsar/observation/PulsarMessageSenderContext.java
Outdated
Show resolved
Hide resolved
...pulsar/src/test/java/org/springframework/pulsar/observation/ObservationIntegrationTests.java
Show resolved
Hide resolved
We have not been checking them in as the build creates them. Do you check them in SI and/or SK? |
I do at the moment. Plus I also added a condition to not run that Gradle task on CI. Although I'll debate with myself if this really has to be deferred to the docs creation phase instead. |
That would be a true optimization (dont' run during build if its 99% static content). However, if the tool picks up changes (would not happen automatically unless version was set to |
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.
Did you see Checkstyle failures:
Error: eckstyle] [ERROR] /home/runner/work/spring-pulsar/spring-pulsar/spring-pulsar/src/main/java/org/springframework/pulsar/observation/PulsarMessageSenderContext.java:74:9: Class MessageHolder should be declared as final. [FinalClass]
?
Yeh I did. I am going to squash/rebase a bit and will address then.
|
7b5993c
to
fd72a3c
Compare
* Observes sends on PulsarTemplate * Observes receives on PulsarListener * Adds auto-generated adocs Closes spring-projects#29
fd72a3c
to
2e7b369
Compare
This adds observations to
PulsarTemplate
andDefaultPulsarMessageListenerContainer
.Remaining items (follow on PRs)
add properties and hook into our Spring Boot starter where it makes sense(Support observations in Spring Boot starter #147)Big thanks to @artembilan and @garyrussell for trailblazing this in
spring-kafka
as this was my reference model for this proposal.@marcingrzejszczak @jonatan-ivanov if either of you get a chance to give this a once-over that would be great. However, I know that treatment was applied to
spring-kafka
and as stated above, this was modeled from that. I am fairly certain there will be no surprises here.