-
Notifications
You must be signed in to change notification settings - Fork 327
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
+ kamon-armeria module #854
base: master
Are you sure you want to change the base?
+ kamon-armeria module #854
Conversation
d3473fd
to
9eaf0cd
Compare
...kamon-armeria/src/main/scala/kamon/armeria/instrumentation/converters/FutureConverters.scala
Outdated
Show resolved
Hide resolved
9eaf0cd
to
e67fd37
Compare
...a/src/main/scala/kamon/armeria/instrumentation/client/ArmeriaHttpClientInstrumentation.scala
Outdated
Show resolved
Hide resolved
...a/src/main/scala/kamon/armeria/instrumentation/client/ArmeriaHttpClientInstrumentation.scala
Outdated
Show resolved
Hide resolved
d80e40b
to
7128c27
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.
A few nitpicks, but otherwise looking good.
Do try and split it into smaller commits in the future!
...tation/kamon-armeria/src/main/scala/kamon/armeria/instrumentation/client/timing/Timing.scala
Outdated
Show resolved
Hide resolved
...a/src/main/scala/kamon/armeria/instrumentation/server/ArmeriaHttpServerInstrumentation.scala
Outdated
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.
Thanks!
I tried this out with
I'll try out the other examples as well, and report back. |
Hey @lucasamoroso, I took a deeper look at this PR today and found three things that are important to address:
Now, we love Armeria and we want this thing merged and ready to roll! So here is what we will be doing in the next couple days:
Looking forward to your ideas and feedback 😃 |
@ivantopo,@SimunKaracic Thanks for the review! |
Hi! |
@ivantopo about:
I'm not sure about moving to a |
93b118a
to
5efb8b5
Compare
Hi!
@ivantopo About this:
I haven't had time to investigate yet but I will. Looking for a feedback about the work done and if there is something more to do in this PR 😄 |
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.
Hey, I have a couple of questions:
- You are storing the context in 4 different places, but never closing the returning scope. Won't that leave dirty threads around?
- You are setting the
CLIENT_TRACE_SCOPE_KEY
in Armerias context, but not reading it from anywhere?
Generally the code is in good shape, but I'd like to answer these before continuing with the PR.
...n-armeria/src/main/java/kamon/armeria/instrumentation/server/ArmeriaHttpServerDecorator.java
Outdated
Show resolved
Hide resolved
instrumentation/kamon-armeria/src/test/scala/utils/ArmeriaServerSupport.scala
Outdated
Show resolved
Hide resolved
The contexts are stored inside a try-with-resources, i understand that the returned scope will be closed after the execution of the try block.
I'm using this in here https://github.com/kamon-io/Kamon/pull/854/files#diff-051d01483bf35bea603907712f76837cc9fd2e519e321daa3bb86229753865dfR49 but i'm not sure it's needed. For sure we need to propagate the Kamon context before call
But when the request is complete:
I'm not sure if it's necessary to run
with Kamon context. As an example, that I hope clarifies what I am trying to say, if i ran this code:
The log line will show that the Kamon context is the same that we've configured in the Armerias context. But if i ran this code
The log line will show that the Kamon context is a different from what we've configured in the Armerias context. In this case the requestHandler was created with a Kamon context |
Hello!! |
Hey @lucasamoroso, I wanted to get into this PR and tried to rebase it to master locally but it didn't go well. There are some minor changes to tests due to the Scalatest version upgrade, but after finishing those changes the tests are not passing anymore 😢 Do you have time to try rebasing and pushing the updates? Otherwise let me know and I can move it forward 💪 |
Hi @ivantopo! Sure I'll be working on this |
4fc74d9
to
e3c33ed
Compare
Hey @ivantopo! I did a rebase and fixed the tests. The problem with the tests was that they must extend from |
Hey @lucasamoroso, thanks for the update! Generally speaking it looks fine and tests pass locally, but I couldn't make it work with one of my Armeria applications by publishing locally and trying it out 🤷 I spent some time trying to debug it and figured out that the
But then I had to patch the server ports for it not to blow up. My guess is that when the bridge is in place the entire ServerBuilder class gets loaded before the advise is applied. The only difference I found is that all Kamon tests are running with Java 8, but the app I'm running is using Java 17. Also tried with Java 11 and it didn't work either. Have you tried it with newer java versions? |
So what is the status of armeria module ? |
97d24da
to
ee37133
Compare
Hello. @ivantopo you wrote that you migrate several services to armeria. Are you going to add kamon supporting ? |
Hey @yarosman. Yes! I'm going to make it my new year's resolution to get this PR merged and released ASAP! We indeed have almost all of our services on Armeria now, but we did manual instrumentation on them long ago so kind of lost the urgency to do this for our own use case. But now is the time 🙏 (I might have said that before, hopefully this will REALLY be the time!) |
@ivantopo, how about adding you to the adopters list and get some goods, then? 😉 line/armeria#4626 |
@ivantopo Hi! Any ETA to merge PR? |
Commented some comments. Lot went on since then - if anybody has some context on those places, would appreciate it - I don't use armeria |
Sadly I don't remember where are we right now, I stopped using Armeria about 2/3 years ago, because I left the company where I was using it, so I also lost track of the new features that Armeria added that could help us to resolve those issues. If we want this feature I could help updating the library and trying to understand where are we right now. |
... |
Hi @yarosman, I'm not sure if this PR will reach master, I'll need to revisit the implementation and also I lost track of the new Armeria features (I'm working with Go now) . |
Armeria http server and client instrumentation.
I tested it manually and added tests based on
okhttp
andakka-http
instrumentation modules tests.Still there is work to do like the https/grpc server instrumention, but i think this PR is a good Armeria module starting point.
Thanks to @dpsoft for his help :)