Several implementation suggestions based on sample project

[Sam-Kruglov]

So, I have finally finished the MVP of what I was trying to do with tracing and it is available here:
https://github.com/Sam-Kruglov/bike-rental-demo

I ask you to take a look and we can discuss whether the way I changed this extension fits your vision or not. When we agree on something I will create a PR. Sorry for not creating a PR right away, that would probably be easier, but it will take time and I already have this working, so while I do that, you can already check it out.

New features

• At every step, we have our Message payload included in the tags of the span.
• All messages are logged:
  • Command payloads at DEBUG
  • User-friendly events at INFO
  • Event payloads at DEBUG
  • Query payloads at TRACE
  • Query return values at TRACE
• Span names actually contain the Message names, like "Send BikeRentCommand" instead of "sendCommandMessage"
• Event publishing is a separate Span with a user-friendly event name.
• All message handler exceptions are caught
  • Expected exceptions are only logged at DEBUG without a stacktrace and reported to the span as a log with an error.expected key.
  • Unexpected exceptions are logged at ERROR with a stacktrace and reported to the span as a log with an error.unexpected key.
• All application logs mentioned above include tracer id.
• Event handling Spans have names that are dynamically resolved with AOP using package names (questionable, but otherwise you only have application name to know the distinguish the projections)

Instructions to run

1. Checkout the master branch
2. Start Jaeger UI (you can use the included docker-componse.yml for that, localhost:16686)
3. Start the app.
4. Use requests.http to fire some commands
5. Look at Jaeger UI

[smcvb]

Thanks for sharing this elaborate demo info @Sam-Kruglov!
As soon as time allows it, we'll check out the repository you've linked towards. 👍

[Sam-Kruglov]

@smcvb Pinging for an update.
Also, more people get confused with the exception logging being missing from the Command and Query handlers. That would probably go to the main repo. We could split my handlers (which do both logging and tracing) into two, put the logging part in the main code and the tracing part here. Try catch would be kinda 80% similar though
see https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!topic/axonframework/k6tb6EKTRxc
and https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!topic/axonframework/NowYBqtyzCk

[smcvb]

I've added a priority of 4, being a 'would', to this issue you've added.
We have quite a lot of things to deal with at the moment, so please have patience.
If it is very, very important for you progress with a specific client, I'd suggest you make direct contact with AxonIQ to increase the priority.

[Sam-Kruglov]

@smcvb thanks! No, it's not very important. Just thought that it might be buried too deep, so decided to remind after a week. I will not remind anymore if everything's on track

[smcvb]

Alright, that's good to hear.
I've put it up in the right place so that we don't forget, so no worries Sam 👍.

[smcvb]

Hi @Sam-Kruglov, it has been some time but we are finally looking to provide a final release of the tracing extension. As such, we are trying to tackle each issue, with yours being one of the last.

This brings me to a request I have for you.
After reading the "new features" section in your main description, I feel some of those have already been included in the past few months.

I would like to ask you to check out the current version and check which things you feel are still missing. For now the "current version" means building your own snapshot, but we will provide a 4.3 release somewhere next week too.

[Sam-Kruglov]

Will take a look

[smcvb]

Thanks for checking it out again and providing numerous fine grained suggestions!
As you've noticed, some have been covered, others have been neglected for the time being.

I am taking the assumption all those suggestions stemmed from this demo project you've shared here, in combination with the current stance of the product. As such, I feel everything this issue states has been covered, making it so this one can be closed. Regardless, let me be complete and go over the points you've shared:

• At every step, we have our Message payload included in the tags of the span.

I'd argue it to be great to allow configurability on what's included in the tags. As it stands, this is all hardwired as you've noticed. Adding the payload as a tag, although easy, sounds like a lot. However, I can imagine there are scenarios to include this. For now I don't see that defaulting to this suggested behaviour is the best way to go though.

• All messages are logged:
  • Command payloads at DEBUG
  • User-friendly events at INFO
  • Event payloads at DEBUG
  • Query payloads at TRACE
  • Query return values at TRACE

I think a well placed LoggingInterceptor, potentially adjusted to chance logging levels, covers this point already.

• Span names actually contain the Message names, like "Send BikeRentCommand" instead of
  "sendCommandMessage"

Great suggestion indeed, something which also came our way from another angle. Hence why this has been implemented following the operation name guidelines.

• Event publishing is a separate Span with a user-friendly event name.

For now we feel that event publication is always paired with another type of message. Let's further debate on this matter in #40 instead of on this issue.

• All message handler exceptions are caught
  • Expected exceptions are only logged at DEBUG without a stacktrace and reported to the span as a log with an error.expected key.
  • Unexpected exceptions are logged at ERROR with a stacktrace and reported to the span as a log with an error.unexpected key.

I'd need to read up on Open Tracing's thought on sharing exceptions through their tags. Regardless, sounds interesting, might justify a dedicated issue to discuss the desired behaviour. From there we could think about a clean implementation.

• All application logs mentioned above include tracer id.

Sensible to include and simple enough to achieve on a users own accord I think. Don't think this requires anything extension specific right now.

• Event handling Spans have names that are dynamically resolved with AOP using package names (questionable, but otherwise you only have application name to know the distinguish the projections)

Interesting way to guarantee a form of uniqueness with the messages. Uncertain whether we should include AOP in this extension just for a span's operation name though.

---

Taking my description on the above pointers, I think we can summarize this issue as an accumulation of thoughts, some which have been resolved, some which will be dismissed and some which would benefit from a dedicated issue. We should make sure to reference this original if issues relating to any of the above will be introduced, as you have already done with a couple. :-)

[Sam-Kruglov]

I'll get back to this when I'm done adapting the code to the latest version

[Sam-Kruglov]

I think I've addressed all of the points I wanted in other issues. Thanks for the effort @smcvb!